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

Store Attributes of Certificates in x509_certs alongside the whole certificate #688

Open
TheSecMaven opened this issue Aug 28, 2021 · 9 comments · May be fixed by #709
Open

Store Attributes of Certificates in x509_certs alongside the whole certificate #688

TheSecMaven opened this issue Aug 28, 2021 · 9 comments · May be fixed by #709

Comments

@TheSecMaven
Copy link
Contributor

What would you like to be added

We can't query smallstep for anything related to certs because the only thing in the DB is the bytes of the cert. Storing the cert alongside more columns like the common name, not after date, and more, would let us enable more complex queries, and optimize performance for future use cases.

Why this is needed

Enable searching of Certificates signed by attributes of the certificate.

@TheSecMaven
Copy link
Contributor Author

Based on the architecture, it could be as simple as a new method in this library that lets you store more things alongside the existing table, and then update the CA to use that new method

if err := db.Set(certsTable, []byte(crt.SerialNumber.String()), crt.Raw); err != nil {

Then would need to add those columns to the create table setup, and run some tests

@dopey dopey transferred this issue from smallstep/nosql Aug 30, 2021
@dopey
Copy link
Contributor

dopey commented Aug 31, 2021

Related #282, #239.

We recently released certificate observability features as part of our hosted product. Open source users can link a standalone CA and monitor generated certificates (https://info.smallstep.com/certificate-manager-early-access-mvp/). We'll be adding enhanced querying, as well as CRL/OCSP in the coming weeks. Linking a single standalone CA is free.

So, with that in mind, our focus, with respect to observability, will be improving that product.

That said, we agree that storing more certificate attributes, so that they can be easily searched without parsing the whole certificate, would be a good thing. And, there's general consensus on enabling a SQL backend so that users can query the DB without writing bespoke ETLs. Since this is part of the product (and there is a free-tier), we're unlikely to focus internal engineering efforts here - at least in the short to mid-term. If folks from the community would like to contribute these types of enhancements, we'd be happy to help answer questions and facilitate development.

@TheSecMaven
Copy link
Contributor Author

Thanks @dopey so what would be acceptable to the team for the free tier?

To be more specific, I think we should store, notAfter, notBefore, all parts of the subject DN (so that you can query on locality separate from organizational unit, for example), SANS, list of non-critical and critical extensions (can query for certs with an extension).

Once we nail down the columns, we can enhance the logic to create the table if it doesn't exist (and also make sure if it does exist the columns match, for existing users)

@dopey
Copy link
Contributor

dopey commented Sep 1, 2021

Our goal is to have everything that you mentioned available in the free tier of the hosted product. But not implemented in the open source. Let me explain a bit more.

The open source has hooks in it that allow us to replace the open source DB with one that implements the same interface. In our hosted stack (closed source) we have implemented that interface in a way that allows us to do observability, reporting, monitoring, and querying over the attributes of a cert. An open source standalone CA will be able to "link" up to our hosted product and take advantage of our proprietary implementation for free (up to a point - if you link more than one authority, you'll have to pay for it).

You're asking about whether it would be possible to improve the DB implementation of the interface that is in open source. I think our answer is "Yes, but we're not going to do it (for now)". Our short to medium term focus is to make the proprietary hosted observability features as good as possible. We want free-tier users to use the hosted version as well. Now, if someone from the community wants to contribute the code that you're talking about we will happily work with them to get it merged.

Does that make sense?

@TheSecMaven
Copy link
Contributor Author

Yeah, I'm asking if someone from the community contributed code that does what I outlined above, would that be acceptable to your team? Want to make sure that what gets done will be accepted generally.

@dopey
Copy link
Contributor

dopey commented Sep 1, 2021

Yep, we're not here to gate-keep useful code being added to the open source. That said, we also won't merge changes that aren't well thought out (both in terms of their execution and their interplay with the existing code base) and tested (@mkkeffeler you've contributed code in the past and understand the process). Every bit of code in the open source is something we are responsible for maintaining. We are reluctant to manage this feature in open source, so the code will need to be robust and have wide support from the community (so that we can rely on the community to maintain it).

tl;dr we welcome PRs here, but we also reserve the right not to merge them if we believe they don't meet our standards or we can't promise to maintain them. If that happens, you may have to be content with compiling your own binary :/

@TheSecMaven
Copy link
Contributor Author

makes complete sense. mostly trying to get a feel for it as well, make sure we are thinking things through. Totally get that supporting it is needed, but I don't think this should be that complex, so will have to see.

@TheSecMaven
Copy link
Contributor Author

I opened an MR with an initial look at this. I think there's an opportunity for a next iteration to enable things like searching by SAN or searching by extension. For that, we would want to establish a one to many relationship so that could work well.

Will do that in a separate MR, but wanted to share here so that it's known.

@maraino
Copy link
Contributor

maraino commented Sep 9, 2021

Hi @mkkeffeler the proper solution for this is to implement the db interfaces in SQL. Mainly this one

certificates/db/db.go

Lines 45 to 57 in 837db2e

type AuthDB interface {
IsRevoked(sn string) (bool, error)
IsSSHRevoked(sn string) (bool, error)
Revoke(rci *RevokedCertificateInfo) error
RevokeSSH(rci *RevokedCertificateInfo) error
GetCertificate(serialNumber string) (*x509.Certificate, error)
StoreCertificate(crt *x509.Certificate) error
UseToken(id, tok string) (bool, error)
IsSSHHost(name string) (bool, error)
StoreSSHCertificate(crt *ssh.Certificate) error
GetSSHHostPrincipals() ([]string, error)
Shutdown() error
}

But these two will be also required:

type DB interface {
CreateProvisioner(ctx context.Context, prov *linkedca.Provisioner) error
GetProvisioner(ctx context.Context, id string) (*linkedca.Provisioner, error)
GetProvisioners(ctx context.Context) ([]*linkedca.Provisioner, error)
UpdateProvisioner(ctx context.Context, prov *linkedca.Provisioner) error
DeleteProvisioner(ctx context.Context, id string) error
CreateAdmin(ctx context.Context, admin *linkedca.Admin) error
GetAdmin(ctx context.Context, id string) (*linkedca.Admin, error)
GetAdmins(ctx context.Context) ([]*linkedca.Admin, error)
UpdateAdmin(ctx context.Context, admin *linkedca.Admin) error
DeleteAdmin(ctx context.Context, id string) error
}

certificates/acme/db.go

Lines 16 to 40 in 837db2e

type DB interface {
CreateAccount(ctx context.Context, acc *Account) error
GetAccount(ctx context.Context, id string) (*Account, error)
GetAccountByKeyID(ctx context.Context, kid string) (*Account, error)
UpdateAccount(ctx context.Context, acc *Account) error
CreateNonce(ctx context.Context) (Nonce, error)
DeleteNonce(ctx context.Context, nonce Nonce) error
CreateAuthorization(ctx context.Context, az *Authorization) error
GetAuthorization(ctx context.Context, id string) (*Authorization, error)
UpdateAuthorization(ctx context.Context, az *Authorization) error
CreateCertificate(ctx context.Context, cert *Certificate) error
GetCertificate(ctx context.Context, id string) (*Certificate, error)
CreateChallenge(ctx context.Context, ch *Challenge) error
GetChallenge(ctx context.Context, id, authzID string) (*Challenge, error)
UpdateChallenge(ctx context.Context, ch *Challenge) error
CreateOrder(ctx context.Context, o *Order) error
GetOrder(ctx context.Context, id string) (*Order, error)
GetOrdersByAccountID(ctx context.Context, accountID string) ([]string, error)
UpdateOrder(ctx context.Context, o *Order) error
}

@TheSecMaven TheSecMaven linked a pull request Sep 21, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants