Unexpected checking of principals

[candlerb]

I am testing pam-ussh. I am logging in to the target machine as user "web", and my certificate has principals ["web@anywhere" "database@anywhere" "root@anywhere"]. This works for ssh login because my sshd_config has:

TrustedUserCAKeys /etc/ssh/trusted_user_ca
AuthorizedPrincipalsCommand /etc/ssh/authprinc.sh %u
AuthorizedPrincipalsCommandUser nobody

and the script /etc/ssh/authprinc.sh is

#!/bin/sh
echo "$1@webserver"
echo "$1@anywhere"

This gives me a way of authorizing access to machines in groups, similar to zones described here.

So far so good. However, pam-ussh refuses to permit sudo. I have configured it with default options:

auth [success=1 default=ignore] pam_ussh.so
auth requisite                  pam_deny.so
auth required                   pam_permit.so

Note in particular that I have not specified authorized_principals or authorized_principals_file, and the documentation says they both default to ""

Initially, pam-ussh logs only "no valid certs found". So I made a small patch to log some more info:

--- a/pam_ussh.go
+++ b/pam_ussh.go
@@ -133,6 +133,11 @@ func authenticate(w io.Writer, uid int, username, ca string, principals map[stri
                in = rest
        }

+       if len(caPubkeys) == 0 {
+               pamLog("No certificate authority keys available.")
+               return AuthError
+       }
+
        c := &ssh.CertChecker{
                IsUserAuthority: func(auth ssh.PublicKey) bool {
                        for _, k := range caPubkeys {
@@ -147,15 +152,18 @@ func authenticate(w io.Writer, uid int, username, ca string, principals map[stri
        for idx := range keys {
                pubKey, err := ssh.ParsePublicKey(keys[idx].Marshal())
                if err != nil {
+                       pamLog("pubkey %d could not be parsed: %v\n", idx, err)
                        continue
                }

                cert, ok := pubKey.(*ssh.Certificate)
                if !ok {
+                       pamLog("pubkey %d is not a certificate\n", idx)
                        continue
                }

                if err := c.CheckCert(username, cert); err != nil {
+                       pamLog("pubkey %d fails CheckCert: %v\n", idx, err)
                        continue
                }

Now what I get is this:

May  2 13:31:55 sagan pam-ussh[2355]: pubkey 0 is not a certificate
May  2 13:31:55 sagan pam-ussh[2355]: pubkey 1 is not a certificate
May  2 13:31:55 sagan pam-ussh[2355]: pubkey 2 is not a certificate
May  2 13:31:55 sagan pam-ussh[2355]: pubkey 3 fails CheckCert: ssh: principal "web" not in the set of valid principals for given certificate: ["web@anywhere" "database@anywhere" "root@anywhere"]
May  2 13:31:55 sagan pam-ussh[2355]: no valid certs found

(My ssh agent has three private keys and one cert)

It appears that ssh.CertChecker.CheckCert is rejecting the certificate on the basis that it requires a particular principal, equal to the logged-in username. Digging further, it's the "username" argument being passed into CheckCert, which is checked against the list of principals in the certificate, if this list is non-empty. This is taking place before pam-ussh does its own principal checks.

As a result, there seems to be a hard-coded policy that if the certificate contains principals, then the bare ssh login name must exist as a principal in that certificate - which unfortunately doesn't work with my scheme for principals.

As far as I can see, this restriction is in the go ssh library, and there's not much that pam-ussh can do about it short of re-implementing the CheckCert function. I could propose a change to the go library here so that if the principal you pass in is empty string, this check is skipped:

	if len(principal) > 0 and len(cert.ValidPrincipals) > 0 {

A better option might be to have a setting for pam-ussh to pass a fixed string, e.g. "sudo" to CheckCert. This would then accept any certificate which includes that principal.

However, assuming that neither of the above happens, I still think it would be worthwhile to:

1. Document this limitation, i.e. that principals are checked even if you don't configure authorized_principals or authorized_principals_file
2. Add a "debug" flag to pam-ussh so that information like that shown above can be printed, to help diagnose such problems

[srstsavage]

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.

[candlerb]

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.

[srstsavage]

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.

[candlerb]

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

[pmoody-]

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 :))

[jessespears]

@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!

[korfuri]

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.

[candlerb]

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.

[korfuri]

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.

[candlerb]

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).