-
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
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.
A PKCS#8 key can also be password-protected. The need of password is not really a question of PEM vs DER vs P12. (We could just say we don't support encrypted private keys)
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’ll add a section saying that this feature won’t support encrypted private keys, and that it’s the credential provider’s responsibility to take care of decryption if necessary.
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.
d1806de adds a section on certificate and key field formats, and specifies that encrypted private keys are not supported
| "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
Eh2406
left a comment
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.
Just some first impressions from a first read.
| "kind":"tls-identity", | ||
| // Registry information (see https://doc.rust-lang.org/cargo/reference/credential-provider-protocol.html#registry-information) | ||
| "registry":{"index-url":"sparse+https://registry-url/index/"}, | ||
| // Additional command-line args (optional) |
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.
What do these args represent? Where do they come from?
|
|
||
| ## Certificate and key formats | ||
|
|
||
| The `certificate` and `key` fields are expected to correspond to the same TLS client identity. If a credential provider is unable to supply a usable client identity, it may return empty fields. |
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.
When should it be returning "empty fields" vs "Err":{"kind":"not-found"}?
|
|
||
| The `certificate` field contains the client certificate chain in PEM format, with newlines escaped using `\n`. If multiple certificates are present, they are expected to be concatenated PEM blocks. | ||
|
|
||
| The `key` field contains the private key corresponding to the client certificate, in PEM format, with newlines escaped using `\n`. |
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.
Every Cryptography expert I've ever talked to has strong opinions about PEM. The fact that it is widely used means that it's widely supported, but that doesn't necessarily mean we should start using it. I would want opinions from Rust Crypto before making this decision.
| Authentication at the TLS level is different from the token-based methods for [Registry Authentication](https://doc.rust-lang.org/cargo/reference/registry-authentication.html) exposed by the [Credential Provider Protocol](https://doc.rust-lang.org/cargo/reference/credential-provider-protocol.html) since it takes place before the connection to the registry is established. It is not currently possible to write a [credential plugin](https://doc.rust-lang.org/cargo/reference/registry-authentication.html#credential-plugins) that enables this type of authentication with a registry, but an extension to that protocol would make this possible. | ||
|
|
||
| # Guide-level explanation | ||
| [guide-level-explanation]: #guide-level-explanation |
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.
What is our guidance on how registries should implement authentication? Should registries use MTLS? Should registries use tokens? Should registries use both? If the answer is, as it probably is, "It's complicated" then what are the factors that would drive that decision?
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