Skip to content
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

x/crypto/ssh: CertChecker.CheckCert ability to check against multiple principals #38829

Open
candlerb opened this issue May 3, 2020 · 2 comments
Open

Comments

@candlerb
Copy link

@candlerb candlerb commented May 3, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

N/A (x/crypto is separate dependency)

What operating system and processor architecture are you using (go env)?

N/A for this issue (Linux x86_64)

What did you do?

I was testing pam-ussh, which uses x/crypto to validate SSH certificates via ssh-agent before deciding to authorize sudo.

The issue as it relates to pam-ussh is described here: uber/pam-ussh#15

pam-ussh has its own logic for checking principals in certificates. However the certificate was being rejected by CheckCert() before it even got to that point.

What's happening is this: pam-ussh calls CheckCert() to see if the certificate is valid. However it has a fixed policy, which is: if the certificate has any principals, then the supplied "principal" string value must appear in that list. Code: https://github.com/golang/crypto/blob/master/ssh/certs.go#L384

In the case of pam-ussh: because you must pass in a principal, it passes in the current username, and this enforces a policy that the bare username must appear as a principal in the certificate.

This is equivalent to the default login policy with OpenSSH if you don't provide an AuthorizedPrincipalsFile or AuthorizedPrincipalsCommand option. However if you do, the given file or script defines a list of possible principals which are permitted to appear in the certificate.

CheckCert() doesn't appear to be able to handle this case: if the certificate has any principals, then only one item can be provided to match against it. If you pass an empty string then it will require the certificate to contain the empty principal (it's unclear if that's even a thing).

What did you expect to see?

The solution I prefer is to have a variant of CheckCert() which takes a slice of principals, instead of a single principal. If the certificate contains any principals, then at least one of them must be in this list. This replicates the checking policy of OpenSSH.

(Aside: this implies that an empty list will never authorize a certificate which contains principals; that is I believe consistent with OpenSSH)

The existing CheckCert() can then become a wrapper around this new function, passing its "principal" argument as a 1-element slice.

Other options I considered:

  • Pass a callback function to CheckCert for checking principals

  • Add another callback function to the CertChecker struct. It would still receive the "principal" argument from CheckCert, but your callback function could treat it differently, e.g. parse it as a comma-separated list. If the function is nil, it would use the current logic.

  • Have a way to disable principal checking altogether in CheckCert, so that the caller can do their own check. Passing empty string for principal might be a way to do that, which would avoid any change to call signatures, although it depends whether certificates with empty principals are a useful "thing" or not. If not, it would simply be:

            if len(principal) > 0 and len(cert.ValidPrincipals) > 0 {
    
@gopherbot gopherbot added this to the Unreleased milestone May 3, 2020
@dmitshur dmitshur changed the title x/crypto: ssh.CertChecker.CheckCert ability to check against multiple principals x/crypto/ssh: CertChecker.CheckCert ability to check against multiple principals May 8, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented May 8, 2020

@candlerb
Copy link
Author

@candlerb candlerb commented Aug 24, 2020

Have a way to disable principal checking altogether in CheckCert, so that the caller can do their own check.

There was a suggestion in uber/pam-ussh#15 (comment) that you can bypass the principal check by passing in the first principal of the certificate itself:

if err := c.CheckCert(cert.ValidPrincipals[0], cert); err != nil {

(or empty string, if the cert has no principals). Then you perform a custom principal check afterwards. If that's considered an acceptable workaround then this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.