Remove nsslib from IPA #367
Conversation
|
ipalib/util.py
Outdated
| # TLSv1 and later is to setup SSLContext with PROTOCOL_SSLv23 | ||
| # and then negate the insecure SSLv2 and SSLv3 | ||
| ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23) | ||
| ctx.options |= (ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3) |
There was a problem hiding this comment.
()are not needed- Python sets these flags and more by default.
There was a problem hiding this comment.
- alright
- I was thinking explicit is better than implicit but I can remove it if you insist
ipalib/util.py
Outdated
| # official Python documentation states that the best option to get | ||
| # TLSv1 and later is to setup SSLContext with PROTOCOL_SSLv23 | ||
| # and then negate the insecure SSLv2 and SSLv3 | ||
| ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23) |
There was a problem hiding this comment.
You want to set secure settings for cipher suites, too. The default settings are rather bad. In fact it would be better if you use ssl.create_default_context instead of ssl.SSLContext.
There was a problem hiding this comment.
I think we dropped create_default_context as it was loading system-wide certs which we do not want.
I did not find any mentions in the official documentation about create_default_context() setting different cipher suites from SSLContext, can you elaborate more on which should be used?
There was a problem hiding this comment.
create_default_context does not load the system certificates if you pass in a cafile, capath or cadata argument. Does it really makes sense to ignore the system's trust store? How are we going to validate certificates from 3rd parties, when NSSDB is gone? FreeIPA supports 3rd party certificates for FreeIPA UI, e.g. Lets Encrypt.
Contrary to SSLContext, create_default_context sets additional options like better selection and order of cipher suites. (https://docs.python.org/3/library/ssl.html#ssl.create_default_context).
The settings are: PROTOCOL_TLS, OP_NO_SSLv2, and OP_NO_SSLv3 with high encryption cipher suites without RC4 and without unauthenticated cipher suites.
There was a problem hiding this comment.
AFAIK we are ignoring system's trust store now so I do not think this should be changed. As for 3rd party certificates, I think all these are store in /etc/ipa/ca.crt so we should be safe there (see ipa_certupdate.py:update_client()).
I am not opposed to using create_default_context(), although I thought we may want to have the SSLContext set up exactly according to our needs and in the same way in all Python versions should this ever change.
ipalib/util.py
Outdated
| ctx.load_verify_locations(cafile) | ||
|
|
||
| if client_certfile is not None: | ||
| ctx.load_cert_chain(client_certfile) |
There was a problem hiding this comment.
Are we using unencrypted client certs? Python's ssl module can handle separate key/cert files and encrypted private keys.
There was a problem hiding this comment.
We are. I guess the question here is whether we'd be able to track it with certmonger.
There was a problem hiding this comment.
I guess so:
Usage: ipa-getcert request [options]
Required arguments:
* If using an NSS database for storage:
-d DIR NSS database for key and cert
-n NAME nickname for NSS-based storage (only valid with -d)
-t NAME optional token name for NSS-based storage (only valid with -d)
* If using files for storage:
-k FILE PEM file for private key
-f FILE PEM file for certificate (only valid with -k)
* If keys are to be encrypted:
-p FILE file which holds the encryption PIN
-P PIN PIN value
There was a problem hiding this comment.
Thanks for the hint, this may work.
There was a problem hiding this comment.
I was thinking about this - do we get anything by encrypting the client cert (ra-agent.pem)? The file is root-readable only, if we encrypt it we're only adding more overhead to reading it as the same user can get to the encryption PIN anyway.
|
You're right, I should probably write some design. The current implementation does not check CRL or OSCP, so we're "fine" with this change. There is a plan on doing CRL check in certmonger, though. |
|
Did you open a bug against NSS or python-nss regarding the PIN requirement? |
|
@rcritten I spoke to the NSS people who assured me it's the intended behavior. But thanks for the remainder, I will open a Bugzilla for that as well, I was considering it before Christmas. edit: https://bugzilla.redhat.com/show_bug.cgi?id=1410143 |
ipaserver/plugins/dogtag.py
Outdated
| self.ca_certificate_nickname = "caCert" | ||
| self._read_password() | ||
| self.ca_cert = paths.IPA_CA_CRT | ||
| self.client_certfile = paths.RA_AGENT_PEM |
There was a problem hiding this comment.
Note to myself: RA_AGENT_PEM will need to move to dot_ipa as well. Testing guide on Wiki will need updating, too.
ipalib/util.py
Outdated
| # official Python documentation states that the best option to get | ||
| # TLSv1 and later is to setup SSLContext with PROTOCOL_SSLv23 | ||
| # and then negate the insecure SSLv2 and SSLv3 | ||
| ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23) |
There was a problem hiding this comment.
create_default_context does not load the system certificates if you pass in a cafile, capath or cadata argument. Does it really makes sense to ignore the system's trust store? How are we going to validate certificates from 3rd parties, when NSSDB is gone? FreeIPA supports 3rd party certificates for FreeIPA UI, e.g. Lets Encrypt.
Contrary to SSLContext, create_default_context sets additional options like better selection and order of cipher suites. (https://docs.python.org/3/library/ssl.html#ssl.create_default_context).
The settings are: PROTOCOL_TLS, OP_NO_SSLv2, and OP_NO_SSLv3 with high encryption cipher suites without RC4 and without unauthenticated cipher suites.
ipalib/util.py
Outdated
| ctx.load_verify_locations(cafile) | ||
|
|
||
| if client_certfile is not None: | ||
| ctx.load_cert_chain(client_certfile) |
There was a problem hiding this comment.
I guess so:
Usage: ipa-getcert request [options]
Required arguments:
* If using an NSS database for storage:
-d DIR NSS database for key and cert
-n NAME nickname for NSS-based storage (only valid with -d)
-t NAME optional token name for NSS-based storage (only valid with -d)
* If using files for storage:
-k FILE PEM file for private key
-f FILE PEM file for certificate (only valid with -k)
* If keys are to be encrypted:
-p FILE file which holds the encryption PIN
-P PIN PIN value
ipalib/util.py
Outdated
| # TLSv1 and later is to setup SSLContext with PROTOCOL_SSLv23 | ||
| # and then negate the insecure SSLv2 and SSLv3 | ||
| ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23) | ||
| ctx.options |= (ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3) |
ipalib/util.py
Outdated
| ctx.options |= version | ||
|
|
||
| if cafile is not None: | ||
| ctx.verify_mode = ssl.CERT_REQUIRED |
There was a problem hiding this comment.
I don't like the fact that no cafile implicitly means no validation. I'd rather have an explicit argument like verify=False with default setting verify=True. By the way is there any use case to have an unverified connection in FreeIPA?
|
stlaz
force-pushed
the
nsslib-removal
branch
3 times, most recently
from
f313a53
to
d423630
Compare
|
In the last update I added SSLv2 support in IPAHTTPSConnection for backward compatibility (https://goo.gl/images/gqh2D9). |
ipalib/util.py
Outdated
|
|
||
| # high ciphers without RC4, MD5, TripleDES, pre-shared key | ||
| # and secure remote password | ||
| ctx.set_ciphers("HIGH:!aNULL:!eNULL:!MD5:!RC4:!3DES:!PSK:!SRP") |
There was a problem hiding this comment.
How about you move the hard coded constants to a config option, e.g. tls_ciphers? It makes it much easier to change the option later or set Fedora crypto policy in /etc/ipa/default.conf.
There was a problem hiding this comment.
I believe that the settings in config options are meant to be user-friendly but I don't think a string such as this one is. I would just set the healthy defaults here and have an admin of the IPA server decide which ciphers should be used by setting these in the config files that are already available.
There was a problem hiding this comment.
No argue on user friendly-ness here. Yes, the cipher string is awkward. The cipher string is used all over the place, e.g. Apache mod_ssl, nginx and tons of other libraries that use OpenSSL. Admins are familiar with it.
ctx.set_ciphers() overrides any default settings in other config files, e.g. system policies. In order to make the ciphers configurable, you have to offer an API.
There was a problem hiding this comment.
This can (and IMO should) be done in a subsequent PR.
|
I created a design for this effort: http://www.freeipa.org/page/V4/Replace_NSS_with_OpenSSL |
|
Wait, you added support for SSLv2? Please remove it, it isn't needed even for backwards compatibility and would not be considered a regression. |
|
@rcritten I wonder if we need to support any version except TLS 1.2 at all. Are there any versions of FreeIPA stack that do not have TLS 1.2 support? |
|
Let's not make @stlaz jump through more bike-shedding hoops. How about we let him finish this PR, and then address TLS versions, ciphers and other simplifications in another PR? |
|
@rcritten |
|
SSLv2 should not be supported, period. Not that it would work anyway because most SSL libs have completely removed this support, but it is just a terrible idea to even try and allow it. The rest I'm flexible on. |
stlaz
force-pushed
the
nsslib-removal
branch
2 times, most recently
from
b587871
to
29381fe
Compare
|
In the latest patchset, the "ipaCert" is removed from the "/etc/httpd/alias/" NSSDB and all the machinery around the certificate is moved accordingly. |
|
All the raised issues should've been addressed in the latest PR. Except for the NSS DB creation, please answer the question in |
stlaz
force-pushed
the
nsslib-removal
branch
2 times, most recently
from
64d6cfc
to
eaf728d
Compare
|
NSS DB creation removed from server install, did not realize it does not matter anymore. |
| if os.path.exists(paths.IPA_RADB_DIR): | ||
| if newdb.has_nickname('ipaCert'): | ||
| if os.path.exists(paths.RA_AGENT_PEM): | ||
| if certdb.has_nickname(ra_nick): |
There was a problem hiding this comment.
This if statement is superfluous - the condition will always be true, because it is verified to be true above.
There was a problem hiding this comment.
I will make it go away.
ipaserver/install/ca.py
Outdated
| @@ -177,14 +177,14 @@ def install_check(standalone, replica_config, options): | |||
| if standalone: | |||
| dirname = dsinstance.config_dirname( | |||
| installutils.realm_to_serverid(realm_name)) | |||
| cadb = certs.CertDB(realm_name, subject_base=options._subject_base) | |||
| cadb = certs.CertDB(realm_name, paths.HTTPD_ALIAS_DIR, | |||
| subject_base=options._subject_base) | |||
There was a problem hiding this comment.
The same NSS database as below in install() should be used here, i.e. paths.PKI_TOMCAT_ALIAS_DIR.
ipaserver/install/httpinstance.py
Outdated
| @@ -420,7 +419,8 @@ def __setup_ssl(self): | |||
|
|
|||
| def __import_ca_certs(self): | |||
| # first for the RA DB | |||
| db = certs.CertDB(self.realm, subject_base=self.subject_base) | |||
| db = certs.CertDB(self.realm, paths.HTTPD_ALIAS_DIR, | |||
| subject_base=self.subject_base) | |||
| self.import_ca_certs(db, self.ca_is_configured) | |||
There was a problem hiding this comment.
I don't think this needs to be done twice.
ipaserver/plugins/rabase.py
Outdated
| self.sec_dir = paths.IPA_RADB_DIR | ||
| self.pwd_file = os.path.join(paths.IPA_RADB_DIR, 'pwdfile.txt') | ||
| self.sec_dir = paths.HTTPD_ALIAS_DIR | ||
| self.pwd_file = os.path.join(paths.HTTPD_ALIAS_DIR, 'pwdfile.txt') |
There was a problem hiding this comment.
This does not make sense anymore, the same as in RestClient.__init__() should be done here.
ipaserver/secrets/store.py
Outdated
| @@ -46,7 +46,7 @@ def PKI_TOMCAT_password_callback(): | |||
|
|
|||
|
|
|||
| def HTTPD_password_callback(): | |||
There was a problem hiding this comment.
Isn't this unused now?
There was a problem hiding this comment.
Did not find it anywhere, will remove it.
|
Upgrade from 4.3 fails with: |
|
CA-less to CA-full |
|
|
stlaz
force-pushed
the
nsslib-removal
branch
from
eaf728d
to
7859073
Compare
|
The issues should hopefully be fixed |
stlaz
force-pushed
the
nsslib-removal
branch
from
7859073
to
5bb660e
Compare
|
Fixed another issue with CA-less to CA-full upgrade. |
|
Upgrade from 4.4.3 asks for a PKCS#12 file password and then fails: |
The "ipaCert" nicknamed certificate is not required to be in /var/lib/ipa/radb NSSDB anymore as we were keeping a copy of this file in a separate file anyway. Remove it from there and track only the file. Remove the IPA_RADB_DIR as well as it is not required anymore. https://fedorahosted.org/freeipa/ticket/5695 https://fedorahosted.org/freeipa/ticket/6680
|
This should now be fixed. In my endless naivety I had thought passing no password to |
|
CA-less to CA-ful conversion still fails: Not sure if it's caused by the PR or not, but either way it can be fixed later. |
|
|
|
@HonzaCholasta I saw this issue as well, once you hit it on a VM no |
|
OK. Let's fix it later. |
This batch of patches removes NSSConnection along with the whole ipapython.nsslib from IPA and replaces it with more standard httplib.HTTPSConnection.
NSSConnection was causing a lot of trouble in the past because it is apparently very fragile when it comes to nss library initialization. On top of that, when NSSConnection is used to set up an HTTPS connection in FIPS, it always requires a password to NSS database as NSS apparently tries to create a temporary private key and store it to the database even though client authentication is not required in the SSL connection.
TODO (will require changes in certmonger/dogatg.c):
https://fedorahosted.org/freeipa/ticket/5695