Unexpected checking of principals about pam-ussh HOT 10 OPEN

How about replacing

if err := c.CheckCert(username, cert); err != nil {

with

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

This seems better than a using fixed string since the cert is guaranteed to contain cert.ValidPrincipals[0], and effectively bypasses checking principals here while still checking:

CriticalOptions, ValidPrincipals, revocation, timestamp and the signature of the certificate.

from pam-ussh.

I don't think that works. The overall test becomes:

• the certificate must include ValidPrincipals[0]; and
• the certificate must include any of ValidPrincipals[0] to ValidPrincipals[len-1]

But that simplifies to:

• the certificate must include ValidPrincipals[0]

i.e. the other members of ValidPrincipals are pointless.

from pam-ussh.

My suggestion is to stop checking principals in ssh.CheckCert by feeding it the first principal in the cert (or an empty string if none are set), but to still do principal validation later in pam-ussh as expected.

I haven’t written a test with this change so I could definitely be missing something obvious.

from pam-ussh.

Oh I understand now, sorry. Yes, that cheat will work, pretty ugly though :-)

from pam-ussh.

pam_ussh.go actually used to do that:

pmoody-@58498a8

I'm trying to remember why I changed this behavior, 3+ years ago now (the internal commit logs probably have more detailed information, but I don't have access to those anymore :))

from pam-ussh.

@pmoody- that internal commit log reads "check cert is good for the user trying to use it", and wasn't reviewed. Your guess is as good as mine!

from pam-ussh.

I think there is a solution here that could resolve multiple concerns at once:

1. I think ussh should not unconditionally require that the local PAM_RUSER username is a principal on the cert. This could be made an option (maybe enabled by default for backward compatibility). This would allow users a lot more flexibility in how they manage their certificates. In particular, it would allow users to forward an agent that only contains the "sudoer" cert if they use ussh with sudo, without forwarding the cert they use to authenticate to sshd (this last option requires a bit of trickery on the client-side, but that's out of scope here).
2. There's no need to patch Go's CertChecker to check that a certificate is valid for multiple principals, one can simply call CheckCert for each of these principals. This seems much better than re-implementing this validation logic in ussh, IMO.
3. To check that a certificate is valid for all principals (i.e. that it lists no principals) one can just check len(cert.ValidPrincipals) == 0 and then call CheckCert as usual with any principal.

I'm going to work on a PR that does this.

from pam-ussh.

a certificate is valid for all principals (i.e. that it lists no principals)

I confirm that what you says is true for CheckCert, but this is a divergence from OpenSSH behaviour. See sshd_config(5) where it says:

Note that certificates that lack a list of principals will not be permitted for authentication using TrustedUserCAKeys.

The only way I know to permit authentication where the certificate has no principals, is to add an entry to ~/.ssh/authorized_keys which specifies cert-authority without any principals="..."

I think it would be very dangerous to accept a certificate without principals as a sort of super-certificate that matches all principals. For example, if you are using Hashicorp Vault to issue certs, the client can request the set of principals to be included, and each one is checked to see if the user is authorized to request it. However they can request no principals - and if no default principal has been set in the signing role, then they'll get a certificate with no principals.

If pam_ussh were to reject certificates with no principals, I'd be much more comfortable with that.

from pam-ussh.

Oh, that is very interesting. The spec and OpenSSH's implementation are disagreeing. The spec says a certificate with no principals should be valid for all principals: https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.certkeys?rev=1.19&content-type=text/x-cvsweb-markup

"valid principals" is a string containing zero or more principals as
strings packed inside it. These principals list the names for which this
certificate is valid; hostnames for SSH_CERT_TYPE_HOST certificates and
usernames for SSH_CERT_TYPE_USER certificates. As a special case, a
zero-length "valid principals" field means the certificate is valid for
any principal of the specified type.

Which I agree is a massive foot-gun given how ssh-keygen will happily create principal-less certs if you omit -n.

(edit: I have tested that OpenSSH does indeed work as documented, too)

Okay, I'll change course and add rejection of principal-less certs to the PR I have in progress.

from pam-ussh.

The spec says a certificate with no principals should be valid for all principals

Thank you for that! I ought to test it with host certificates too, i.e. whether a host certificate with no principals acts as a "wildcard" that matches all hosts (and would be equally dangerous).

from pam-ussh.