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

[NET-8091] Add file-system-certificate config entry for API gateway #20873

Merged
merged 21 commits into from
Apr 15, 2024

Conversation

nathancoleman
Copy link
Member

@nathancoleman nathancoleman commented Mar 18, 2024

Description

Today, inline-certificate is the only supported way of specifying a TLS certificate + private key for an api-gateway. As the name suggests, the certificate + private key values are "inlined" in the configuration entry.

This PR adds a new file-system-certificate configuration entry that instead accepts file paths to a certificate + private key that are accessible in the local file system of the api-gateway at runtime. This functions in the same way that terminating-gateway does today (docs) but adds support for rotation by watching the file system for changes to the referenced certificate + private key.

Ideally, we will be able to add this same support for rotation in terminating-gateway in the future as rotating the certificate and private key today require a restart of the terminating-gateway, which is not ideal (docs).

Testing & Reproduction steps

Testing on VMs

Check out this repo from @nathancoleman

  1. Follow the directions in the README.
  2. You can check the secret at http://localhost:19000/config_dump under dynamic_active_secrets.
image
  1. echo secret_val | base64 -d and you should see the value of cert.crt

  2. Replace the value of cert.crt with this certificate:

-----BEGIN CERTIFICATE-----
MIICnTCCAkOgAwIBAgIRALTzFaRsW70V2hhpw6XdmKEwCgYIKoZIzj0EAwIwgbkx
CzAJBgNVBAYTAlVTMQswCQYDVQQIEwJDQTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNj
bzEaMBgGA1UECRMRMTAxIFNlY29uZCBTdHJlZXQxDjAMBgNVBBETBTk0MTA1MRcw
FQYDVQQKEw5IYXNoaUNvcnAgSW5jLjFAMD4GA1UEAxM3Q29uc3VsIEFnZW50IENB
IDMwNzU0NzMzNjE5NTQ2OTU4MjIxMTI5MjE3NzM2MzUxMzM4MDc0NzAeFw0yNDA0
MDkxODIxNDNaFw0yNTA0MDkxODIxNDNaMBwxGjAYBgNVBAMTEXNlcnZlci5kYzEu
Y29uc3VsMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAESdiYfmA1LURAFLGt59hu
U3UgTMcSAIf3Hl+KYrDRWStjviB4ueemTI+8pXjd8gETXKxp+i5vxIBF9bW95zSb
2KOBxzCBxDAOBgNVHQ8BAf8EBAMCBaAwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsG
AQUFBwMCMAwGA1UdEwEB/wQCMAAwKQYDVR0OBCIEIJ32ryqpgyOlctlfqniwLBCt
HosjcftQwK1rjoI6LFnCMCsGA1UdIwQkMCKAICJ9E1X61c/+gkiHJmmiQkVx3g89
MkOYHfJlRoTQlpNpMC0GA1UdEQQmMCSCEXNlcnZlci5kYzEuY29uc3Vsgglsb2Nh
bGhvc3SHBH8AAAEwCgYIKoZIzj0EAwIDSAAwRQIhAKvSL4YDwQPMNgGMibtRTYRs
cHZzR4Vv1JN6D2FDmVYUAiBT+6ySH0bXK9/kFA+RkTbPUwDzfe+qlzs+frK5tqE+
Dw==
-----END CERTIFICATE--
  1. Then check the value of the dynamic_active_secrets again, it should match the above cert.

Testing on K8S

You cannot do the same type of replacement for K8S, so if you would like to test K8S check out the K8S PR. You will mainly verify that the certificate is a file-system-certificate as opposed to an inline-certificate.

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@github-actions github-actions bot added theme/api Relating to the HTTP API interface theme/envoy/xds Related to Envoy support labels Mar 18, 2024
agent/xds/secrets.go Outdated Show resolved Hide resolved
.changelog/20873.txt Outdated Show resolved Hide resolved
@missylbytes missylbytes marked this pull request as ready for review April 10, 2024 14:55
@missylbytes missylbytes requested a review from a team as a code owner April 10, 2024 14:55
@missylbytes missylbytes requested review from a team, andrewstucki and sarahalsmiller and removed request for a team April 10, 2024 14:59
agent/xds/secrets.go Outdated Show resolved Hide resolved
case structs.InlineCertificate:
if cert, ok := cfgSnap.APIGateway.Certificates.Get(certRef); ok {
certs = append(certs, cert)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intentionally omit this check in both cases?

if !ok {
    continue
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes because I think this would only be a programmer error, not something that people should run into.

Copy link
Member Author

Choose a reason for hiding this comment

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

they're functionally the same unless I'm missing something.

if ok {
 // do something
}

vs.

if !ok {
  continue
}

// do something

Copy link
Contributor

@NiniOak NiniOak Apr 15, 2024

Choose a reason for hiding this comment

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

I guess it just depends on the behaviour we want

if ok { 
    // do something 
}

using the above code, we successfully retrieve the certificate and move on to the next thing.

if !ok {
  continue
}
// do something

This code here indicates that what we'll like to do if the cert isn't found, we can skip the rest of the iteration and move onto the next thing. Just depends on how we want to handle it. I saw that we used if !ok { continue} in the previous implementation and was wondering why we didn't here.

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhhh I see what you're saying. I just went with the current flavor because it simplified the code and had the same outcome, no more reasoning that

Copy link
Contributor

@NiniOak NiniOak left a comment

Choose a reason for hiding this comment

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

Small error check comment but mostly looks good. The work done here is MASSIVE and IMPRESSIVE. Very well done 🚀

@missylbytes missylbytes requested a review from NiniOak April 15, 2024 19:53
@missylbytes missylbytes merged commit 5e9f02d into main Apr 15, 2024
85 of 86 checks passed
@missylbytes missylbytes deleted the file-system-certificate branch April 15, 2024 20:45
missylbytes added a commit that referenced this pull request Apr 18, 2024
…20873)

* Define file-system-certificate config entry

* Collect file-system-certificate(s) referenced by api-gateway onto snapshot

* Add file-system-certificate to config entry kind allow lists

* Remove inapplicable validation

This validation makes sense for inline certificates since Consul server is holding the certificate; however, for file system certificates, Consul server never actually sees the certificate.

* Support file-system-certificate as source for listener TLS certificate

* Add more required mappings for the new config entry type

* Construct proper TLS context based on certificate kind

* Add support or SDS in xdscommon

* Remove unused param

* Adds back verification of certs for inline-certificates

* Undo tangential changes to TLS config consumption

* Remove stray curly braces

* Undo some more tangential changes

* Improve function name for generating API gateway secrets

* Add changelog entry

* Update .changelog/20873.txt

Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>

* Add some nil-checking, remove outdated TODO

* Update test assertions to include file-system-certificate

* Add documentation for file-system-certificate config entry

Add new doc to nav

* Fix grammar mistake

* Rename watchmaps, remove outdated TODO

---------

Co-authored-by: Melisa Griffin <melisa.griffin@hashicorp.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
missylbytes added a commit that referenced this pull request Apr 18, 2024
…20873)

* Define file-system-certificate config entry

* Collect file-system-certificate(s) referenced by api-gateway onto snapshot

* Add file-system-certificate to config entry kind allow lists

* Remove inapplicable validation

This validation makes sense for inline certificates since Consul server is holding the certificate; however, for file system certificates, Consul server never actually sees the certificate.

* Support file-system-certificate as source for listener TLS certificate

* Add more required mappings for the new config entry type

* Construct proper TLS context based on certificate kind

* Add support or SDS in xdscommon

* Remove unused param

* Adds back verification of certs for inline-certificates

* Undo tangential changes to TLS config consumption

* Remove stray curly braces

* Undo some more tangential changes

* Improve function name for generating API gateway secrets

* Add changelog entry

* Update .changelog/20873.txt

Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>

* Add some nil-checking, remove outdated TODO

* Update test assertions to include file-system-certificate

* Add documentation for file-system-certificate config entry

Add new doc to nav

* Fix grammar mistake

* Rename watchmaps, remove outdated TODO

---------

Co-authored-by: Melisa Griffin <melisa.griffin@hashicorp.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
missylbytes added a commit that referenced this pull request Apr 18, 2024
…20873)

* Define file-system-certificate config entry

* Collect file-system-certificate(s) referenced by api-gateway onto snapshot

* Add file-system-certificate to config entry kind allow lists

* Remove inapplicable validation

This validation makes sense for inline certificates since Consul server is holding the certificate; however, for file system certificates, Consul server never actually sees the certificate.

* Support file-system-certificate as source for listener TLS certificate

* Add more required mappings for the new config entry type

* Construct proper TLS context based on certificate kind

* Add support or SDS in xdscommon

* Remove unused param

* Adds back verification of certs for inline-certificates

* Undo tangential changes to TLS config consumption

* Remove stray curly braces

* Undo some more tangential changes

* Improve function name for generating API gateway secrets

* Add changelog entry

* Update .changelog/20873.txt

Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>

* Add some nil-checking, remove outdated TODO

* Update test assertions to include file-system-certificate

* Add documentation for file-system-certificate config entry

Add new doc to nav

* Fix grammar mistake

* Rename watchmaps, remove outdated TODO

---------

Co-authored-by: Melisa Griffin <melisa.griffin@hashicorp.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
@missylbytes
Copy link
Contributor

This will not be back-ported at this time.

krastin added a commit that referenced this pull request Apr 22, 2024
krastin added a commit that referenced this pull request Apr 22, 2024
* Initial rebrand for HCP Terraform

* Apply suggestions from code review

Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com>

* path fix and redirect

* reintroduce nav from #20873 and #20994

---------

Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com>
Co-authored-by: Krastin Krastev <krastin@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/api Relating to the HTTP API interface theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants