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

Add per-issuer AIA URI information to PKI secrets engine #16563

Merged
merged 11 commits into from
Aug 19, 2022

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Aug 3, 2022

Per discussion on GitHub with @maxb, this allows issuers to have their
own copy of AIA URIs. Because each issuer has its own URLs (for CA and
CRL access), its necessary to mint their issued certs pointing to the
correct issuer and not to the global default issuer. For anyone using
multiple issuers within a mount, this change allows the issuer to point
back to itself via leaf's AIA info.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>


  • Tests for per-issuer AIA info
  • Docs updates
  • Changelog
Siteproxy
@cipherboy cipherboy added this to the 1.12.0-rc1 milestone Aug 3, 2022
@cipherboy cipherboy requested review from kitography, stevendpclark and a team August 3, 2022 20:22
@cipherboy cipherboy changed the title Add per-issuer AIA URI information Add per-issuer AIA URI information to PKI secrets engine Aug 3, 2022
@cipherboy cipherboy force-pushed the cipherboy-per-issuer-aia-handling branch from 7ab6961 to 7b97305 Compare August 3, 2022 20:34
@cipherboy cipherboy force-pushed the cipherboy-per-issuer-aia-handling branch from 7b97305 to 5d648c4 Compare August 4, 2022 15:56
@cipherboy cipherboy requested a review from taoism4504 as a code owner August 4, 2022 15:56
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

A couple of nits, nothing preventing merging

builtin/logical/pki/path_fetch_issuers.go Show resolved Hide resolved
builtin/logical/pki/path_fetch_issuers.go Outdated Show resolved Hide resolved
leafCert = parseCert(t, resp.Data["certificate"].(string))
require.Equal(t, leafCert.IssuingCertificateURL, []string{"https://example.com/ca"})
require.Equal(t, leafCert.OCSPServer, []string{"https://example.com/ocsp"})
require.Equal(t, leafCert.CRLDistributionPoints, []string{"https://example.com/crl"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we exercise the PATCH version in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you want more PATCH tests. :-)

@maxb
Copy link
Contributor

maxb commented Aug 12, 2022

Hi, thanks for working on this, and sorry it has taken me so long to look into the code.

There's a bit of a terminology mix-up here - AIA is a technical term which refers only to Vault's issuing_certificates setting. It's inaccurate to use it as umbrella terminology covering the CRL and OCSP settings too.

Looking further into the rest of the code too now.

website/content/api-docs/secret/pki.mdx Show resolved Hide resolved
Comment on lines +1934 to +2014
distribution point. These separate CRLs should either be aggregated into a
single CRL (externally; as Vault does not support this functionality)
or multiple `crl_distribution_points` should be specified here, pointing
to each cluster and issuer.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is orthogonal to this PR, as this is language copied from elsewhere in the docs, but actually specifying multiple crl_distribution_points is not a correct way to deal with Performance Replication environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah......

builtin/logical/pki/storage.go Show resolved Hide resolved
builtin/logical/pki/path_intermediate.go Outdated Show resolved Hide resolved
@@ -674,7 +674,8 @@ func generateCert(sc *storageContext,
data.Params.PermittedDNSDomains = input.apiData.Get("permitted_dns_domains").([]string)

if data.SigningBundle == nil {
// Generating a self-signed root certificate
// Generating a self-signed root certificate. Since we have no
// issuer entry yet, we default to the global URLs.
entries, err := getURLs(ctx, sc.Storage)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this approach works for now, but it means there are certain configurations it will be hard to orchestrate reasonably with desired state configuration e.g. terraform-provider-vault. (I'm thinking of ones where you deliberately want to create multiple issuers with different URLs.)

It may be worth re-visiting allowing the URLs to be passed in as part of the root generation request itself in a later PR.

Copy link
Contributor Author

@cipherboy cipherboy Aug 15, 2022

Choose a reason for hiding this comment

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

I think there should be no issue once terraform-provider-vault is updated to support setting AIAs on issuers? (cc @stevendpclark & @benashz).

My understanding from the RFC -- and correct me if I'm wrong -- is generally you don't want AIAs on roots. So while we don't support setting them on root generation (as you mention in the last sentence), the approach you'd probably want is:

  1. Generate root without AIA URLs.
  2. Configure per-issuer AIA on the root.
  3. Generate intermediates. These will use the root's per-issuer AIA for signing the intermediate cert proper.
  4. Once intermediate is imported, configure per-issuer AIA on the intermediate.
  5. Issue leaf certs, which will consume intermediate AIAs.
  6. Global URLs should probably never be set. This workflow works independent of whether one or several mounts are used.

AIA should then be structured correctly, minus the cross-cluster discussion elsewhere.


This comment is mostly about preserving backwards compatibility and noting why we call getURLs rather than the newer issuer-entry call (used above).

builtin/logical/pki/path_fetch_issuers.go Show resolved Hide resolved
builtin/logical/pki/path_fetch_issuers.go Outdated Show resolved Hide resolved
builtin/logical/pki/path_fetch_issuers.go Outdated Show resolved Hide resolved
@cipherboy
Copy link
Contributor Author

@maxb said:

There's a bit of a terminology mix-up here - AIA is a technical term which refers only to Vault's issuing_certificates setting. It's inaccurate to use it as umbrella terminology covering the CRL and OCSP settings too.

https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.2.1 is titled AIA and includes both CA Issuers and OCSP. That CRL isn't strictly included there is, IMO, more a comment towards its structure (instead having a separate extension) -- but it is mentioned from there as being related conceptually. :-)

Indeed, I think they kinda intended to if you read this quote from the start (+/- actually having it as a separate extension):

  • The Authority Information Access (AIA) CRL extension, as
    specified in [RFC4325], was added as Section 5.2.7.

My 2c. but I think conceptually, it makes sense especially given OCSP is actually part of the main AIA extension.

@maxb
Copy link
Contributor

maxb commented Aug 15, 2022

@maxb said:

There's a bit of a terminology mix-up here - AIA is a technical term which refers only to Vault's issuing_certificates setting. It's inaccurate to use it as umbrella terminology covering the CRL and OCSP settings too.

https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.2.1 is titled AIA and includes both CA Issuers and OCSP. That CRL isn't strictly included there is, IMO, more a comment towards its structure (instead having a separate extension) -- but it is mentioned from there as being related conceptually. :-)

Ah... whoops. My experience is entirely with CRL-based revocation, never having actually deployed OCSP, and I extrapolated wrongly. OK then, I just learned that OCSP URLs are communicated within AIA too!

Indeed, I think they kinda intended to if you read this quote from the start (+/- actually having it as a separate extension):

  • The Authority Information Access (AIA) CRL extension, as
    specified in [RFC4325], was added as Section 5.2.7.

My 2c. but I think conceptually, it makes sense especially given OCSP is actually part of the main AIA extension.

I don't think your quote from the changelog compared to the previous version of the RFC - mentioning the AIA extension for CRLs, not certificates - gives any additional clarity.

I do now see two of the three URLs are actually in the AIA extension, and the third can be considered "AIA adjacent" from a certain point of view - and maybe that's sufficient to use AIA as an unofficial catch-all term in internal program symbols.

In public documentation and error messages, though, I think there's still a good case for talking about "AIA/CDP" or "Authority Information Access or CRL Distribution Points", so that users receive unambiguous information, regardless of their viewpoint regarding implied nesting of the concepts, or not.

@cipherboy
Copy link
Contributor Author

Cool, I'll update much of the wording to be clearer but otherwise leave them internally as AIAs. Thanks!

@cipherboy cipherboy force-pushed the cipherboy-per-issuer-aia-handling branch from 5d648c4 to ec68a76 Compare August 15, 2022 22:10
Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

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

All of these are totally nits, feel free to ignore. This is really well done.

builtin/logical/pki/storage.go Outdated Show resolved Hide resolved
builtin/logical/pki/path_root.go Outdated Show resolved Hide resolved
builtin/logical/pki/path_root.go Outdated Show resolved Hide resolved
builtin/logical/pki/backend_test.go Outdated Show resolved Hide resolved
@cipherboy cipherboy force-pushed the cipherboy-per-issuer-aia-handling branch from 0134187 to 8812163 Compare August 18, 2022 21:10
Per discussion on GitHub with @maxb, this allows issuers to have their
own copy of AIA URIs. Because each issuer has its own URLs (for CA and
CRL access), its necessary to mint their issued certs pointing to the
correct issuer and not to the global default issuer. For anyone using
multiple issuers within a mount, this change allows the issuer to point
back to itself via leaf's AIA info.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Also add it to the considerations page as something to watch out for.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This introduces a common helper per Steve's suggestion.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
This clarifies which request parameter the invalid URL is contained
in, disambiguating the sometimes ambiguous usage of AIA, per suggestion
by Max.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy force-pushed the cipherboy-per-issuer-aia-handling branch from 8812163 to a5e743c Compare August 18, 2022 22:15
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

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

This looks great!

@stevendpclark stevendpclark merged commit e1b9e9b into main Aug 19, 2022
@cipherboy cipherboy deleted the cipherboy-per-issuer-aia-handling branch December 1, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants