SERVER-46275 Add ECDSA w/OCSP support to cert gen tool #1351
Conversation
jstests/ssl/x509/mkcert.py
Outdated
| open(extfile, 'wt').write('basicConstraints=CA:FALSE\n') | ||
| open(extfile, 'at').write('subjectAltName=DNS:localhost,IP:127.0.0.1\n') | ||
| open(extfile, 'at').write('subjectKeyIdentifier=hash\n') | ||
| key_usage = ('keyUsage=nonRepudiation,digitalSignature,keyEncipherment\n' if "responder" in cert['name'] |
There was a problem hiding this comment.
Should these extensions be listed in certs.yml ?
There was a problem hiding this comment.
I think long term it would be ideal if we could define ECDSA certs much like we define RSA certs. And I admit that hard coding can be inconvenient e.g. hard-coding the AIA extensions means that once we get the OCSP responder working without the trailing slashes, we'll need to make a small change to this script to remove the trailing slashes from the AIA section when generating ECDSA OCSP certs. For this PR though, I would rather minimize the amount of changes needed and keep ECDSA special cased, such that the only thing we define in the cert.yml file is their name, description, issuer, and (now) tags.
If desired, I'd be happy to create a ticket to track the feature of allowing us to define ECDSA certs in the same fashion as RSA certs :)
jstests/ssl/x509/mkcert.py
Outdated
| MUST_STAPLE_VALUE = str('DER:30:03:02:01:05').encode('utf-8') | ||
| MUST_STAPLE_KEY_STR = '1.3.6.1.5.5.7.1.24' | ||
| MUST_STAPLE_KEY = bytes(MUST_STAPLE_KEY_STR, "utf-8") | ||
| MUST_STAPLE_VALUE_STR ='DER:30:03:02:01:05' |
There was a problem hiding this comment.
I know this commit isn't adding it, but could either of you document what SEQ: [ 1, NULL ] actually signifies? (Also that I'm decoding that correctly?)
There was a problem hiding this comment.
Added some clarifying comments!
jstests/ssl/x509/mkcert.py
Outdated
| extfile = tempfile.mkstemp()[1] | ||
| open(extfile, 'wt').write('subjectAltName=DNS:localhost,DNS:127.0.0.1') | ||
| open(extfile, 'wt').write('basicConstraints=CA:FALSE\n') |
There was a problem hiding this comment.
This template is getting big enough we might consider putting it in an external file which we load from here.
There was a problem hiding this comment.
Noted! I've left it the way it is for now since other drivers work is more pressing at the moment, but if you feel strongly about making this optimization now as part of this PR, I can look into it :)
| @@ -558,7 +558,7 @@ certs: | |||
| - 'trusted-ca.pem' | |||
There was a problem hiding this comment.
I think the other parts of this file need to be tagged as well, like the other ECDSA certs and the other OCSP certs.
There was a problem hiding this comment.
I've tagged the other ECDSA certs and added checks for the ECDSA tag. Should the RSA OCSP certs be updated with a tags field if the tags only apply to special cases handling i.e. ECDSA certs? I'm inclined to say no, since they perform no useful function at the moment.
There was a problem hiding this comment.
It's also worth noting that the current code leaves the previous ECDSA tree untouched: they still retain their explicit text header and the private keys are not converted into PKCS8 format as I did not know what those certs were being used for.
jstests/ssl/x509/mkcert.py
Outdated
| cert_filename = make_filename({'name': f"{filename}.crt"}) | ||
| open(cert_filename, 'wt').write(open(temp_cert_filename, 'rt').read()) | ||
| key_filename = make_filename({'name': f"{filename}.key"}) | ||
| open(key_filename, 'wt').write(open(temp_key_filename, 'rt').read()) |
There was a problem hiding this comment.
This line does not close either of the two files that it opens. Consider using shutil.copyfile instead (https://docs.python.org/2/library/shutil.html#shutil.copyfile).
jstests/ssl/x509/mkcert.py
Outdated
| extended_key_usage = ('extendedKeyUsage=serverAuth,clientAuth,OCSPSigning\n' | ||
| if (cert.get('tags') and "responder" in cert['tags']) | ||
| else 'extendedKeyUsage=serverAuth,clientAuth\n') | ||
| open(extfile, 'at').write(extended_key_usage) |
There was a problem hiding this comment.
Instead of the individual open calls that do not close the file (and do not guarantee that the data has been written at all or before the next open call happens), the writes will be better expressed with a scope:
with open(extfile, 'wt') as f: f.write(...) # ... f.write(...)
There was a problem hiding this comment.
Ah thanks! I've taken your suggestion :)
vincentkam
force-pushed
the
mkcert-ecdsa
branch
from
dfcdcb6 to
3f3dfa2
Compare
|
Rebased and squashed! I don't have merge permissions, so I leave the rest to @shreyaskalyan 😸 |
vincentkam
force-pushed
the
mkcert-ecdsa
branch
from
3f3dfa2 to
6c54780
Compare
|
@shreyaskalyan I've added one more commit to strip the trailing slashes from the ECDSA endpoints (and regenerate the certs), per https://jira.mongodb.org/browse/SERVER-46413. |
|
Merged here: 8289739 |
https://jira.mongodb.org/browse/SERVER-46275