Fetch CRLs from a user defined URL #17136

sgmiller · 2022-09-14T17:13:37Z

Extends path_crl to accept either a CRL in PEM or DER format or a URL for the CRL Distribution Point. In the latter case, we periodically fetch that CRL according to it's validity dates.

builtin/credential/cert/backend.go

builtin/credential/cert/path_crls.go

… as trusted certs for CRL fetch

builtin/credential/cert/path_crls.go

stevendpclark

We need to update https://github.com/hashicorp/vault/blob/main/website/content/docs/auth/cert.mdx as part of this as there is an explicit call out

(Note: Vault does not fetch CRLs; the CRLs themselves and any updates must be pushed into Vault when desired, such as via a cron job that fetches them from the source and pushes them into Vault.)

builtin/credential/cert/path_crls.go

sgmiller · 2022-09-15T16:15:11Z

We need to update https://github.com/hashicorp/vault/blob/main/website/content/docs/auth/cert.mdx as part of this as there is an explicit call out

(Note: Vault does not fetch CRLs; the CRLs themselves and any updates must be pushed into Vault when desired, such as via a cron job that fetches them from the source and pushes them into Vault.)

Fixed.

Co-authored-by: Steven Clark <[email protected]>

… into cert-auth-crl-autofetch

stevendpclark · 2022-09-15T17:08:41Z

builtin/credential/cert/path_crls.go

+		}
+
+		b.crlUpdateMutex.RLock()
+		if crl, ok := b.crls[name]; ok {


I'm a bit puzzled by this, nothing will use the crl we are populating within this lock and it would be reset anyways by the setCRL within the fetchCRL call

Also note a make fmt is needed. Everything else looks good though!

If the CRL already existed and it's valid until was in the future, it would be a no-op in fetchCRL. So we wouldn't validate the URL and wouldn't setCRL.

Ah this is tricky, I like what you're doing here: re-setting the same URL on the same existing CRL name will allow users to force a re-fetch of the CRL. I like it! (I was going to ask elsewhere how we'd refresh an existing CRL manually, but this does answer it).

But shouldn't this be a write lock in that case, strictly?

Eugh, fetchCRL doesn't look at the crl.CDP.ValidUntil value, updateCRLs does? So right now we will always reload the URL data?

In particular, fetchCRL takes a new reference, not the modified value here in b.crls. I think Steve is correct, and while we later need a write lock (see my other comment), we don't need the read lock here. And we don't need the entire if block.

Ah gotcha, now that it's straight to fetchURL there's no conditional. Removed the if block and had earlier redone the locking.

cipherboy · 2022-09-15T17:37:28Z

builtin/credential/cert/backend.go

+	return fmt.Errorf("unexpected response code %d fetching CRL from %s", response.StatusCode, crl.CDP.Url)
+}
+
+func (b *backend) updateCRLs(ctx context.Context, req *logical.Request) error {


This needs to hold a write lock, in order for us to safely iterate over b.crls -- and eventually update it. A read lock isn't sufficient (even with a duplication) as we can modify the contents in setCRL or pathCRLWrite during this I think and deadlock.

I think what needs to happen is the lock needs to be propagated up out of setCRL (which'll assume it is locked) and its callers will instead acquire that correctly (i.e., if they're going to call updateCRLs or fetchCRL eventually, they'll just proactively acquire the write lock).

Agreed. Something did feel off about that.

@sgmiller -- @stevendpclark pointed out you didn't add lock here in the periodic func ;-)

This is actually the race test failure:

Failed === RUN TestCRLFetch ================== WARNING: DATA RACE Write at 0x00c002bf1440 by goroutine 9745: runtime.mapassign_faststr() /usr/local/go/src/runtime/map_faststr.go:203 +0x0 github.com/hashicorp/vault/builtin/credential/cert.(*backend).setCRL() /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls.go:270 +0x226 github.com/hashicorp/vault/builtin/credential/cert.(*backend).fetchCRL() /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/backend.go:87 +0x28c github.com/hashicorp/vault/builtin/credential/cert.(*backend).pathCRLWrite() /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls.go:233 +0xe50 github.com/hashicorp/vault/builtin/credential/cert.TestCRLFetch() /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls_test.go:151 +0x1df4 testing.tRunner() /usr/local/go/src/testing/testing.go:1446 +0x216 testing.(*T).Run.func1() /usr/local/go/src/testing/testing.go:1493 +0x47 Previous read at 0x00c002bf1440 by goroutine 9746: runtime.mapiterinit() /usr/local/go/src/runtime/map.go:815 +0x0 github.com/hashicorp/vault/builtin/credential/cert.(*backend).updateCRLs() /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/backend.go:93 +0xaa github.com/hashicorp/vault/builtin/credential/cert.(*backend).updateCRLs-fm() <autogenerated>:1 +0x64 github.com/hashicorp/vault/builtin/credential/cert.TestCRLFetch.func1() /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls_test.go:41 +0x1e6 Goroutine 9745 (running) created at: testing.(*T).Run() /usr/local/go/src/testing/testing.go:1493 +0x75d testing.runTests.func1() /usr/local/go/src/testing/testing.go:1846 +0x99 testing.tRunner() /usr/local/go/src/testing/testing.go:1446 +0x216 testing.runTests() /usr/local/go/src/testing/testing.go:1844 +0x7ec testing.(*M).Run() /usr/local/go/src/testing/testing.go:1726 +0xa84 main.main() _testmain.go:95 +0x2e9 Goroutine 9746 (running) created at: github.com/hashicorp/vault/builtin/credential/cert.TestCRLFetch() /home/circleci/go/src/github.com/hashicorp/vault/builtin/credential/cert/path_crls_test.go:36 +0x385 testing.tRunner() /usr/local/go/src/testing/testing.go:1446 +0x216 testing.(*T).Run.func1() /usr/local/go/src/testing/testing.go:1493 +0x47 ==================

cipherboy

Test and locking updates look good, thanks Scott!

cipherboy · 2022-09-16T12:55:54Z

builtin/credential/cert/path_crls.go

@@ -42,10 +49,13 @@ using the same name as specified here.`,
 	}
 }

-func (b *backend) populateCRLs(ctx context.Context, storage logical.Storage) error {
+func (b *backend) populateCRLsWithoutLock(ctx context.Context, storage logical.Storage) error {


Not to nitpick what is otherwise a good PR, but perhaps we want to call this populateCRLsGrabbingLocks? Just found Without rather confusing the first time when it is used elsewhere.

Changed to lockThenPopulateCRLs

builtin/credential/cert/backend.go

cipherboy

Thanks!!

sgmiller added 2 commits September 14, 2022 12:13

Fetch CRLs from a user defined CDP (PoC)

5adba17

Handle no param sent

89eea4e

cipherboy reviewed Sep 14, 2022

View reviewed changes

builtin/credential/cert/backend.go Outdated Show resolved Hide resolved

cipherboy reviewed Sep 14, 2022

View reviewed changes

builtin/credential/cert/path_crls.go Show resolved Hide resolved

sgmiller added 2 commits September 14, 2022 14:29

Move CRL fetch to a periodFunc. Use configured CA certs + system root…

50ffb09

… as trusted certs for CRL fetch

comments

8407a80

sgmiller requested a review from a team September 14, 2022 19:42

sgmiller changed the title ~~Fetch CRLs from a user defined CDP (PoC)~~ Fetch CRLs from a user defined CDP Sep 14, 2022

changelog

a2cfb64

sgmiller requested a review from cipherboy September 14, 2022 19:45

stevendpclark reviewed Sep 14, 2022

View reviewed changes

builtin/credential/cert/path_crls.go Show resolved Hide resolved

stevendpclark reviewed Sep 14, 2022

View reviewed changes

sgmiller added 3 commits September 14, 2022 15:13

Just use root trust

ce31db1

cdp->url in api

249b17d

Store CRL and populate it initially in cdlWrite

b3e140d

sgmiller requested a review from stevendpclark September 14, 2022 20:27

sgmiller changed the title ~~Fetch CRLs from a user defined CDP~~ Fetch CRLs from a user defined URL Sep 14, 2022

Merge remote-tracking branch 'origin/main' into cert-auth-crl-autofetch

211dcbd

stevendpclark reviewed Sep 15, 2022

View reviewed changes

builtin/credential/cert/path_crls.go Outdated Show resolved Hide resolved

stevendpclark reviewed Sep 15, 2022

View reviewed changes

builtin/credential/cert/path_crls.go Outdated Show resolved Hide resolved

sgmiller added 2 commits September 15, 2022 11:13

Merge remote-tracking branch 'origin/main' into cert-auth-crl-autofetch

343540a

Update docs

89b29ea

sgmiller requested a review from taoism4504 as a code owner September 15, 2022 16:15

sgmiller and others added 3 commits September 15, 2022 11:16

Update builtin/credential/cert/path_crls.go

281fd20

Co-authored-by: Steven Clark <[email protected]>

Handle pre-verification of a CRL url better

1ba0658

Merge branch 'cert-auth-crl-autofetch' of github.com:/hashicorp/vault…

b1de605

… into cert-auth-crl-autofetch

stevendpclark reviewed Sep 15, 2022

View reviewed changes

just in case

b6f382c

cipherboy reviewed Sep 15, 2022

View reviewed changes

sgmiller added 3 commits September 15, 2022 14:11

Fix crl write locking

ad2ce35

Add a CRL fetch unit test

bdb5740

Remove unnecessary validity clear

99c267f

cipherboy approved these changes Sep 16, 2022

View reviewed changes

stevendpclark reviewed Sep 16, 2022

View reviewed changes

builtin/credential/cert/backend.go Outdated Show resolved Hide resolved

sgmiller added 4 commits September 16, 2022 15:34

Better func name

0606bc3

Don't exit early updating CRLs

1114974

lock in updateCRLs

6087cbd

gofumpt

0fae99e

sgmiller requested a review from stevendpclark September 16, 2022 21:12

stevendpclark reviewed Sep 16, 2022

View reviewed changes

builtin/credential/cert/backend.go Outdated Show resolved Hide resolved

err-

4c75900

stevendpclark approved these changes Sep 16, 2022

View reviewed changes

cipherboy approved these changes Sep 16, 2022

View reviewed changes

sgmiller merged commit f5cc167 into main Sep 16, 2022

sgmiller deleted the cert-auth-crl-autofetch branch September 16, 2022 21:44

cipherboy mentioned this pull request Sep 16, 2022

Fix race in cert auth tests #17181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch CRLs from a user defined URL #17136

Fetch CRLs from a user defined URL #17136

sgmiller commented Sep 14, 2022 •

edited

Loading

stevendpclark left a comment

sgmiller commented Sep 15, 2022

stevendpclark Sep 15, 2022

sgmiller Sep 15, 2022

cipherboy Sep 15, 2022 •

edited

Loading

stevendpclark Sep 15, 2022

cipherboy Sep 15, 2022 •

edited

Loading

sgmiller Sep 15, 2022

cipherboy Sep 15, 2022 •

edited

Loading

sgmiller Sep 15, 2022

sgmiller Sep 15, 2022

cipherboy Sep 16, 2022

cipherboy Sep 16, 2022

sgmiller Sep 16, 2022

cipherboy left a comment

cipherboy Sep 16, 2022

sgmiller Sep 16, 2022

sgmiller Sep 16, 2022

cipherboy left a comment

Fetch CRLs from a user defined URL #17136

Fetch CRLs from a user defined URL #17136

Conversation

sgmiller commented Sep 14, 2022 • edited Loading

stevendpclark left a comment

Choose a reason for hiding this comment

sgmiller commented Sep 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy left a comment

Choose a reason for hiding this comment

sgmiller commented Sep 14, 2022 •

edited

Loading

cipherboy Sep 15, 2022 •

edited

Loading

cipherboy Sep 15, 2022 •

edited

Loading

cipherboy Sep 15, 2022 •

edited

Loading