certificates icon indicating copy to clipboard operation
certificates copied to clipboard

[Bug]: inconsistent nkey format in revoked_x509_certs table

Open DonOtuseGH opened this issue 3 years ago • 8 comments

Steps to Reproduce

When revoking a certificarte using step client with mTLS (e.g. step ca revoke --cert cert.pem --key cert.key ...), the serial number of the revoked cert is stored in decimal format in the nkey column in revoked_x509_certs table. It's the same format used in x509_certs table and seems to be the standard/default one.

When the certificate is revoked using step client and the certificates serial number (e.g. step ca revoke 9de5d852dda5c2869fe45d8336df008e ...), the serial number of the revoked cert is stored in hexadecimal format in the nkey column in revoked_x509_certs table.

It seems that the serial number is taken as-is / as it was given during the certificate revoke operation and that it is not normalized before beeing stored in the database. (https://github.com/smallstep/certificates/issues/718)

$ psql -qtnA -d "${dbConn}" -c "select encode(nkey, 'escape')::text from x509_certs;"
209882218884616561056554940555404968078
236816592456780696721919387019098694597
141597777458191518252926419788696651342
164251378795451772745182356178908542026
123632492247848079217495460536869063423
191988272827305694745870852859674624533

$ psql -qtnA -d "${dbConn}" -c "select encode(nkey, 'escape')::text from revoked_x509_certs;"
7B91AA35C41D2DA56E2A4E3799F4F44A
191988272827305694745870852859674624533
123632492247848079217495460536869063423
6A86BD6E727E27742AEB1D7D5F58064E
f28b0ea8b52648a920729f56a40e5395
9de5d852dda5c2869fe45d8336df008e

Your Environment

  • docker image smallstep/step-ca:0.20.0
  • docker image smallstep/step-cli:0.20.0

Expected Behavior

The same serial number format in all tables would be appreciated. Decimal format seems to be the preferred/default/standard one...

Actual Behavior

revoked_x509_certs table uses mixed serial number formats (as is / as provided during revoke operation)

Additional Context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

DonOtuseGH avatar Jul 07 '22 14:07 DonOtuseGH

Hey @DonOtuseGH thanks for opening the issue! You are correct that we take the value of the serial number as it is passed in and then use that value as the key.

I'm trying to work out the most straightforward "fix" here - would it be ok to assume that if a user is going to enter a non-base 10 number that they also enter a prefix indicating the base? That way I could use something like this to parse the value: https://stackoverflow.com/questions/46783352/string-to-big-int-in-go, and then just convert to base 10 afterwards.

dopey avatar Jul 07 '22 20:07 dopey

Hi @dopey, of course this would be a possible solution. But what i think is more important is the matching against the table containing the already issued/signed certificates. This would be another important and IMHO mandatory validation of the input.

DonOtuseGH avatar Jul 12 '22 15:07 DonOtuseGH

The design for revoking a cert does not assume that the cert is stored in the DB. If you obliterate the DB and then start a new DB, you should still be able to revoke an active certificate, even if there is no record of that certificate being created (at least with the current design).

dopey avatar Jul 12 '22 17:07 dopey

@dopey, thanks for your feedback. I know that Smallstep has a slightly different approach regarding CA and certificates than we do in our current use case 😄 However, I have a different opinion regarding data management/database.... For us it is important that there is an up2date CRL and a working OCSP service. The basis for this is the information of the tables x509_certs and revoked_x509_certs of the currently used postgres db backend. So for us it is not an option that this data might not be available someday, on the contrary, it must be highly available and a working backup must exist. Therefore my suggestion to validate the serial number of a certificate to be validated against the data in the table x509_certs. Should this absolutely violate the current concept/intention of Smallstep, it would at least be desirable to keep the serial numbers in both tables in a uniform format.

Thanks and BR

DonOtuseGH avatar Jul 12 '22 19:07 DonOtuseGH

@DonOtuseGH of course, and we appreciate you opening the issue!

I've spoken with the team and we'd prefer that this not be a breaking change - meaning we don't want to only revoke certs that exist in the DB by default. I could see us accepting a feature to only revoke serial numbers that can be found, but that would need to be a flag to the CLI and additional attribute to the API.

Out of curiosity, for your use case, is the cert not available locally to do revocation over mTLS (passing the cert and key instead of the serial)? That would be a way to ensure the certificate does exist.

dopey avatar Jul 13 '22 20:07 dopey

https://github.com/smallstep/cli/issues/698

dopey avatar Jul 13 '22 20:07 dopey

Out of curiosity, for your use case, is the cert not available locally to do revocation over mTLS (passing the cert and key instead of the serial)? That would be a way to ensure the certificate does exist.

Hehe, that's a very good and indeed a valid question! ;-) We use templates for the different certificates types, through which we customize the certificate extensions. According to the policy the server certificates are only allowed to use the extKeyUsage serverAuth extension, so it doesn't work to revoke these certificates with step cli via mTLS ;-)

2022/07/14 10:54:55 /usr/local/go/src/net/http/server.go:3195: http: TLS handshake error from 0.0.0.0:61064: tls: failed to verify client certificate: x509: certificate specifies an incompatible key usage

DonOtuseGH avatar Jul 14 '22 10:07 DonOtuseGH

Yep, that's a great reason.

We recently changed the API for renew to accept the certificate in a token (instead of over mTLS). If we expanded this functionality to revoke the operation would not require client auth.

https://github.com/smallstep/cli/issues/701

dopey avatar Jul 14 '22 20:07 dopey