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

Fetch CRLs from a user defined URL #17136

Merged
merged 23 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions builtin/credential/cert/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,15 @@ package cert

import (
"context"
"crypto/x509"
"fmt"
"io"
"net/http"
"strings"
"sync"
"time"

"github.com/hashicorp/go-multierror"
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/logical"
)
Expand All @@ -14,7 +20,7 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend,
if err := b.Setup(ctx, conf); err != nil {
return nil, err
}
if err := b.populateCRLs(ctx, conf.StorageView); err != nil {
if err := b.lockThenpopulateCRLs(ctx, conf.StorageView); err != nil {
return nil, err
}
return b, nil
Expand All @@ -36,9 +42,10 @@ func Backend() *backend {
pathCerts(&b),
pathCRLs(&b),
},
AuthRenew: b.pathLoginRenew,
Invalidate: b.invalidate,
BackendType: logical.TypeCredential,
AuthRenew: b.pathLoginRenew,
Invalidate: b.invalidate,
BackendType: logical.TypeCredential,
PeriodicFunc: b.updateCRLs,
}

b.crlUpdateMutex = &sync.RWMutex{}
Expand All @@ -63,6 +70,40 @@ func (b *backend) invalidate(_ context.Context, key string) {
}
}

func (b *backend) fetchCRL(ctx context.Context, storage logical.Storage, name string, crl *CRLInfo) error {
response, err := http.Get(crl.CDP.Url)
if err != nil {
return err
}
if response.StatusCode == http.StatusOK {
body, err := io.ReadAll(response.Body)
if err != nil {
return err
}
certList, err := x509.ParseCRL(body)
if err != nil {
return err
}
crl.CDP.ValidUntil = certList.TBSCertList.NextUpdate
return b.setCRL(ctx, storage, certList, name, crl.CDP)
}
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 {
Copy link
Contributor

@cipherboy cipherboy Sep 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Something did feel off about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
==================

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

b.crlUpdateMutex.Lock()
defer b.crlUpdateMutex.Unlock()
var errs *multierror.Error
for name, crl := range b.crls {
if crl.CDP != nil && time.Now().After(crl.CDP.ValidUntil) {
if err := b.fetchCRL(ctx, req.Storage, name, &crl); err != nil {
errs = multierror.Append(errs, err)
}
}
}
return errs.ErrorOrNil()
}

const backendHelp = `
The "cert" credential provider allows authentication using
TLS client certificates. A client connects to Vault and uses
Expand Down
101 changes: 77 additions & 24 deletions builtin/credential/cert/path_crls.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@ package cert
import (
"context"
"crypto/x509"
"crypto/x509/pkix"
"fmt"
"math/big"
url2 "net/url"
"strings"
"time"

"github.com/fatih/structs"
"github.com/hashicorp/vault/sdk/framework"
Expand All @@ -24,11 +27,15 @@ func pathCRLs(b *backend) *framework.Path {

"crl": {
Type: framework.TypeString,
Description: `The public certificate that should be trusted.
Description: `The public CRL that should be trusted to attest to certificates' validity statuses.
May be DER or PEM encoded. Note: the expiration time
is ignored; if the CRL is no longer valid, delete it
sgmiller marked this conversation as resolved.
Show resolved Hide resolved
using the same name as specified here.`,
},
"url": {
Type: framework.TypeString,
Description: `The URL of a CRL distribution point. Only one of 'crl' or 'url' parameters should be specified.`,
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
Expand All @@ -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) lockThenpopulateCRLs(ctx context.Context, storage logical.Storage) error {
b.crlUpdateMutex.Lock()
defer b.crlUpdateMutex.Unlock()
return b.populateCRLs(ctx, storage)
}

func (b *backend) populateCRLs(ctx context.Context, storage logical.Storage) error {
if b.crls != nil {
return nil
}
Expand Down Expand Up @@ -129,7 +139,7 @@ func (b *backend) pathCRLDelete(ctx context.Context, req *logical.Request, d *fr
return logical.ErrorResponse(`"name" parameter cannot be empty`), nil
}

if err := b.populateCRLs(ctx, req.Storage); err != nil {
if err := b.lockThenpopulateCRLs(ctx, req.Storage); err != nil {
return nil, err
}

Expand Down Expand Up @@ -160,7 +170,7 @@ func (b *backend) pathCRLRead(ctx context.Context, req *logical.Request, d *fram
return logical.ErrorResponse(`"name" parameter must be set`), nil
}

if err := b.populateCRLs(ctx, req.Storage); err != nil {
if err := b.lockThenpopulateCRLs(ctx, req.Storage); err != nil {
return nil, err
}

Expand Down Expand Up @@ -188,44 +198,86 @@ func (b *backend) pathCRLWrite(ctx context.Context, req *logical.Request, d *fra
if name == "" {
return logical.ErrorResponse(`"name" parameter cannot be empty`), nil
}
crl := d.Get("crl").(string)
if crlRaw, ok := d.GetOk("crl"); ok {
crl := crlRaw.(string)
certList, err := x509.ParseCRL([]byte(crl))
if err != nil {
return logical.ErrorResponse(fmt.Sprintf("failed to parse CRL: %v", err)), nil
}
if certList == nil {
return logical.ErrorResponse("parsed CRL is nil"), nil
}

certList, err := x509.ParseCRL([]byte(crl))
if err != nil {
return logical.ErrorResponse(fmt.Sprintf("failed to parse CRL: %v", err)), nil
}
if certList == nil {
return logical.ErrorResponse("parsed CRL is nil"), nil
}
b.crlUpdateMutex.Lock()
defer b.crlUpdateMutex.Unlock()
err = b.setCRL(ctx, req.Storage, certList, name, nil)
if err != nil {
return nil, err
}
} else if urlRaw, ok := d.GetOk("url"); ok {
url := urlRaw.(string)
if url == "" {
return logical.ErrorResponse("empty CRL url"), nil
}
_, err := url2.Parse(url)
if err != nil {
return logical.ErrorResponse("invalid CRL url: %v", err), nil
}

if err := b.populateCRLs(ctx, req.Storage); err != nil {
return nil, err
b.crlUpdateMutex.Lock()
defer b.crlUpdateMutex.Unlock()

cdpInfo := &CDPInfo{
Url: url,
}
err = b.fetchCRL(ctx, req.Storage, name, &CRLInfo{
CDP: cdpInfo,
})
if err != nil {
return nil, err
}
sgmiller marked this conversation as resolved.
Show resolved Hide resolved
} else {
return logical.ErrorResponse("one of 'crl' or 'url' must be provided"), nil
}

b.crlUpdateMutex.Lock()
defer b.crlUpdateMutex.Unlock()
return nil, nil
}

func (b *backend) setCRL(ctx context.Context, storage logical.Storage, certList *pkix.CertificateList, name string, cdp *CDPInfo) error {
if err := b.populateCRLs(ctx, storage); err != nil {
return err
}

crlInfo := CRLInfo{
CDP: cdp,
Serials: map[string]RevokedSerialInfo{},
}
for _, revokedCert := range certList.TBSCertList.RevokedCertificates {
crlInfo.Serials[revokedCert.SerialNumber.String()] = RevokedSerialInfo{}

if certList != nil {
for _, revokedCert := range certList.TBSCertList.RevokedCertificates {
crlInfo.Serials[revokedCert.SerialNumber.String()] = RevokedSerialInfo{}
}
}

entry, err := logical.StorageEntryJSON("crls/"+name, crlInfo)
if err != nil {
return nil, err
return err
}
if err = req.Storage.Put(ctx, entry); err != nil {
return nil, err
if err = storage.Put(ctx, entry); err != nil {
return err
}

b.crls[name] = crlInfo
return err
}

return nil, nil
type CDPInfo struct {
Url string `json:"url" structs:"url" mapstructure:"url"`
ValidUntil time.Time `json:"valid_until" structs:"valid_until" mapstructure:"valid_until"`
}

type CRLInfo struct {
CDP *CDPInfo `json:"cdp" structs:"cdp" mapstructure:"cdp"`
Serials map[string]RevokedSerialInfo `json:"serials" structs:"serials" mapstructure:"serials"`
}

Expand All @@ -237,10 +289,11 @@ Manage Certificate Revocation Lists checked during authentication.

const pathCRLsHelpDesc = `
This endpoint allows you to create, read, update, and delete the Certificate
Revocation Lists checked during authentication.
Revocation Lists checked during authentication, and/or CRL Distribution Point
URLs.

When any CRLs are in effect, any login will check the trust chains sent by a
client against the submitted CRLs. Any chain containing a serial number revoked
client against the submitted or retrieved CRLs. Any chain containing a serial number revoked
by one or more of the CRLs causes that chain to be marked as invalid for the
authentication attempt. Conversely, *any* valid chain -- that is, a chain
in which none of the serials are revoked by any CRL -- allows authentication.
Expand Down
Loading