-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Cargo mTLS registry authentication #3907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| // Response kind: this was a TLS client identity request | ||
| "kind":"tls-identity", | ||
| // Base64 byte buffer containing the binary content of your client certificate (empty if unset) | ||
| "cert_blob":"aGVsbG8...gd29ybGQ=", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would supporting raw public keys be an option? That way a registry could just store the public key as a token in the database and doesn't need to parse a client certificate and do a whole bunch of checks to see if the certificate is valid, making it much more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This RFC is intended to allow Cargo to present client certificates, not change how the certificates are processed or validated by the server.
In my use case, a registry is being reverse-proxied by a web server like Nginx, Apache, HAproxy, etc... to provide additional features like SSO, SCIM and LDAP in addition to client identity verification.
Using raw public keys wouldn’t support this reverse-proxy use case, since it would require changes to how the proxy validates the client certificates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean having the option for the credential provider to provide raw public keys in case a registry wants to support this for robustness or any other reason. If you use client certificates to authenticate specific clients rather than just any client who gets a certificate from the CA, then whichever side channel is used to communicate the identity of the client from the reverse-proxy to the registry can pass a the raw public key too, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I was misunderstanding your previous comment. I think we could add an additional field that allows that, what did you have in mind?
| // The format of your client certificate. With the current curl-based backend, supported formats are “PEM”, “DER”, and "P12" (empty for backend default) | ||
| "cert_type":"PEM", | ||
| // Base64 byte buffer containing the binary content of your private key (empty if unset) | ||
| "key_blob":"aGVsbG8...gd29ybGQ=", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already mentioned this on Zulip and probably should be omitted in the first version, but it might be nice to support credential providers that can't export the private key but rather act as a signing oracle to support for example TPM or smartcard backed certificates. Rustls should support that through the SigningKey trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would definitely be a nice use case to support, and as you point out, it looks like rustls has support for this, and I also believe libcurl can be persuaded into doing this.
I don’t know enough about how these signing request flows work to propose a sensible protocol, so that’s mainly why I’ve omitted it here. I’d be happy to consider adding it to this RFC, but also agree that it’s a somewhat independent feature that could be added in a later version.
| // Base64 byte buffer containing the binary content of your client certificate (empty if unset) | ||
| "cert_blob":"aGVsbG8...gd29ybGQ=", | ||
| // The format of your client certificate. With the current curl-based backend, supported formats are “PEM”, “DER”, and "P12" (empty for backend default) | ||
| "cert_type":"PEM", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need to support PKCS#12 format certificates? The credential provider could just convert it to a PEM certificate + separate private key, right? If you are using PKCS#12 to support private key encryption, the credential provider did have to parse the PKCS#12 file anyway to decrypt the private key as cargo doesn't know the password.
Also I don't think there should be a backend dependent default for the type. Either it should be automatically detected from the file header or it should be always explicitly specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think we need to support PKCS#12, it was listed as a format that libcurl already supports so I listed it too.
If we’re fine explicitly specifying the format a PEM certificate + separate signing key would make sense. It can be on the credential provider to convert objects into that format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for libcurl to load a password-protected P12 file you'll have to supply CURLOPT_KEYPASSWD as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3a16df3 now specifies that the certificates and keys are in PEM format
| "kind":"tls-identity", | ||
| // Client certificate chain in PEM format (empty if unset) | ||
| "certificate":"-----BEGIN CERTIFICATE----- | ||
| [Base64 encoded client certificate data] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have to use the \n escape for new lines in json, you can't just have a literal newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ae27ce7 specifies that newlines have to be escaped
This is an RFC aimed at allowing Cargo to present client certificates when forming HTTP connections and support mutual TLS authentication with registries.
It is aimed at forming consensus around the discussion in rust-lang/cargo#10641.
Rendered