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

Vault PKI cert source #315

Closed
wants to merge 4 commits into from

Conversation

pschultz
Copy link
Member

@pschultz pschultz commented Jul 1, 2017

POC for the Vault PKI source. I have not tested this thoroughly yet, especially under load. I want to get feedback on the design first.

I moved the Vault client bits out of the existing vault source, so they can be shared with the new PKI source. I had to redo the token renewal, since that was triggered as part of the refresh logic. However, the PKI source will for the most part just sit and wait for certificates to expire. So I created a wrapper around the *api.Client, that will renew tokens after half their lifetime. This is the same behaviour as in Nomad and Consul Template, for instance.

I left a few TODO comments in the code. I'd appreciate your thoughts on those.

@pschultz pschultz changed the title Vault pki cert source Vault PKI cert source Jul 1, 2017
@magiconair
Copy link
Contributor

What I understand is that you are doing two things here:

  1. refactor the existing code to allow for dynamic cert generation
  2. add vault pki as a dynamic cert generator

What I usually do in these cases is to split the change into two commits: first the functional equivalent refactoring, then add the new feature on top of that new functionality.

I like the Issuer interface but the first thing that came to mind is whether the cert store setup could be extended in general with different kind of issuers. Static onces, dynamic ones, ... That was the idea of the Store which would become a Cache. The other thing I was thinking of when I read this was what about LetsEncrypt?

Please note that this isn't fully thought out but it might make sense to look at the architecture of the cert stores as a whole and not just for Vault.

As a benefit I've learned about golang.org/x/sync/singleflight :)

One other thing caught my eye: the setting of StrictMatch in main.go if the source is an issuer. I think this should be caught during config loading and tested there and this should trigger an INFO message in the log.

@pschultz
Copy link
Member Author

pschultz commented Jul 7, 2017

I will re-arrange the commits to keep the Vault stuff separate, and at the same time move the StrictMatch check into the config package. While it is nice that this check can now be tested, it also means that it will have to be done for all future Issuer implementations (which is easily forgotten).

The store is unaware of any sources (and, in fact, the sources are unaware of the store). The only part that is aware of both is cert.TLSConfig. In my opionion that's a good thing.

Adding other on-demand sources doesn't require more changes to either the store or cert.TLSConfig, just a new Issuer implementation and config support. I threw together two more examples in this gist - please handle with care; it compiles if you drop it in certs, but I haven't checked it for correctness or anything. One is for the ACME protocol (e.g. Letsencrypt) and the other takes a CA cert file and private key and issues certs directly.

The ACMESource in particular would require lots of configuration, and each challenge provider you'd want to support comes with its own rabbit hole, but none of that relates to the store I think.

By the way, as far as I know Letsencrypt will only allow one non-revoked certificate per common name at any given time, so it has to be revoked before a new certificate can be issued. If more than one fabio instance serve a domain they have to coordinate accordingly. That's out of scope of this MR though I think.

It's apparent to me now that a Store.ReplaceCertificate method would be useful. Then the Issuers don't have to keep track of all the certs they ever issued. Apart from that I don't see any beneficial changes I could make to the store.

@pschultz pschultz force-pushed the vault-pki-cert-source branch from 8c3b787 to 2a9e7f1 Compare July 7, 2017 14:28
@magiconair
Copy link
Contributor

@pschultz This has fallen a bit through the cracks for various reasons. I'd like to pick this one up again and merge it if you still feel comfortable about this. Could you rebase this, please?

@pschultz pschultz force-pushed the vault-pki-cert-source branch from 3342d2e to dfe5543 Compare October 10, 2017 07:43
@pschultz
Copy link
Member Author

Rebased on master.

Support certificate sources that load (or issue) certificates on-demand.
Sources that wish to do so implement the new cert.Issuer interface.
Add a new cert.Source, VaultPKISource, that issues certficates on demand
from a HashiCorp Vault PKI backend.
@pschultz pschultz force-pushed the vault-pki-cert-source branch from dfe5543 to e79e187 Compare October 10, 2017 07:47
{
"checksumSHA1": "VhUZFUuhLFSBFUfskMC4am5RIdc=",
"path": "golang.org/x/sync/singleflight",
"revision": "8e0aa688b654ef28caa72506fa5ec8dba9fc7690",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The very last entry in this list is new (singleflight), everything else only re-formatted. Did you change the vendor tool?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written a tool https://github.com/magiconair/vendorfmt to reformat the JSON to a more merge-friendly single-line format. If you run make vendorfmt it should format it.

@magiconair
Copy link
Contributor

Lets merge this.

@magiconair
Copy link
Contributor

Fixes #135

@magiconair
Copy link
Contributor

Thanks a lot @pschultz !

@magiconair
Copy link
Contributor

@pschultz The VaultPKI integration fails with go1.10rc1. I'm getting this. Would you be OK having a look at this?

$ go test -run TestVaultPKISource ./cert
--- FAIL: TestVaultPKISource (2.49s)
	source_test.go:305: Starting vault: "\x1b[0;0mVault v0.9.3 ('5acd6a21d5a69ab49d0f7c0bf540123a9b2c696d')\x1b[0m\n"
    --- FAIL: TestVaultPKISource/renewable_token (0.46s)
    	source_test.go:564: got Get https://127.0.0.1:49363: x509: certificate signed by unknown authority want nil
    --- FAIL: TestVaultPKISource/non-renewable_token (0.26s)
    	source_test.go:564: got Get https://127.0.0.1:49370: x509: certificate signed by unknown authority want nil
    --- FAIL: TestVaultPKISource/renewable_orphan_token (0.30s)
    	source_test.go:564: got Get https://127.0.0.1:49378: x509: certificate signed by unknown authority want nil
    --- FAIL: TestVaultPKISource/non-renewable_orphan_token (0.36s)
    	source_test.go:564: got Get https://127.0.0.1:49390: x509: certificate signed by unknown authority want nil
    --- FAIL: TestVaultPKISource/renewable_wrapped_token (0.34s)
    	source_test.go:564: got Get https://127.0.0.1:49400: x509: certificate signed by unknown authority want nil
    --- FAIL: TestVaultPKISource/non-renewable_wrapped_token (0.29s)
    	source_test.go:564: got Get https://127.0.0.1:49413: x509: certificate signed by unknown authority want nil
FAIL
FAIL	github.com/fabiolb/fabio/cert	2.507s

@pschultz
Copy link
Member Author

pschultz commented Feb 2, 2018

Will do, but not before the 12th because I'm off on vacation.

@magiconair
Copy link
Contributor

@pschultz I've found it. See #434

@pschultz pschultz deleted the vault-pki-cert-source branch May 17, 2018 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants