#282 very basic implementation of custom specified path for idp certificate
Opened 2 years ago by webrat. Modified 2 years ago
webrat/ipsilon master  into  master

file modified
+15 -4

@@ -21,6 +21,7 @@ 

  from datetime import timedelta

  import lasso

  import os

+ import sys

  import time

  import uuid

  

@@ -287,7 +288,6 @@ 

          )

          if cherrypy.config.get('debug', False):

              import logging

-             import sys

              logger = logging.getLogger('lasso')

              lh = logging.StreamHandler(sys.stderr)

              logger.addHandler(lh)

@@ -506,6 +506,10 @@ 

                                   METADATA_DEFAULT_VALIDITY_PERIOD))

          group.add_argument('--saml2-session-dburl',

                             help='session database URL')

+         group.add_argument('--saml2-cert-path', default=None,

+                            help='full path to certificate')

+         group.add_argument('--saml2-key-path', default=None,

+                            help='full path to key')

  

      def configure(self, opts, changes):

          if opts['saml2'] != 'yes':

@@ -516,9 +520,16 @@ 

          if not os.path.exists(path):

              os.makedirs(path, 0700)

  

-         # Use the same cert for signing and ecnryption for now

-         cert = Certificate(path)

-         cert.generate('idp', opts['hostname'])

+         # Use the same cert for signing and encryption for now

+         if opts['saml2_cert_path'] and opts['saml2_key_path']:

+             cert = Certificate(opts['saml2_cert_path'])

+             cert.key = opts['saml2_key_path']

Is there a specific reason you don't use Certificate.import_cert, which would set both properties with a single call?

+             cert.cert = opts['saml2_cert_path']

+         elif any([opts['saml2_cert_path'], opts['saml2_key_path']]):

+             sys.exit('You need to specify both or none of --saml2-cert-path and --saml2-key-path')

+         else:

+             cert = Certificate(path)

+             cert.generate('idp', opts['hostname'])

  

          # Generate Idp Metadata

          proto = 'https'

things one might want to add:
- check if file exists
- check if file is correctly encoded
- test in general

maybe fixes https://pagure.io/ipsilon/issue/19

2 new commits added

  • Merge branch 'master' of ssh://pagure.io/forks/webrat/ipsilon
  • very basic implementation of custom specified path
2 years ago

If the user provides --saml2-cert-path but not --saml2-key-path, this will raise a KeyError. It might be nice to add something to validate that providing one requires the other, with a friendly error message when there's a problem so the traceback is avoided.

I recommend tests, but LGTM.

1 new commit added

  • add a little bit of opts checking
2 years ago

added a little bit of checking.
wasn't able to get the test env running here @puiterwijk probably wanted to add some tests

1 new commit added

  • remove double import of sys
2 years ago

Is there a specific reason you don't use Certificate.import_cert, which would set both properties with a single call?

Sorry for the delay in reviewing this. I've commented with a single question, but I'm not sure that this is enough.
The code should probably have an import_external_cert function that will set the self.cert and self.key, but also copy that cert and key to the install path.
The problem if you don't do that is that you have no idea where the certificate might be stored, so the user setting it up (root) might have access to it, but the ipsilon user might not, either due to file ACLs or selinux.