PR#282: very basic implementation of custom specified path for idp certificate - ipsilon

		`@@ -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']`
alert("boo! (puiterwijk was here)"); Uiterwijk (puiterwijk)">puiterwijk commented 6 years ago 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'`

webrat commented 6 years ago

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

6 years ago

bowlofeggs commented on line 22 of ipsilon/providers/saml2idp.py 6 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.

bowlofeggs commented 6 years ago

I recommend tests, but LGTM.

1 new commit added

add a little bit of opts checking

6 years ago

webrat commented 6 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

6 years ago

puiterwijk commented on line 38 of ipsilon/providers/saml2idp.py 6 years ago

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

puiterwijk commented 6 years ago

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.