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

Issue #2700: Adds support for X509 and JWT specific SVID TTLs #3445

Merged
merged 6 commits into from
Oct 26, 2022

Conversation

dennisgove
Copy link
Contributor

Fixes #2700
Depends on spiffe/spire-api-sdk#29

This change adds support for X509 and JWT specific SVID TTLs in each of the following places

  • Default values in spire-server configuration. Similar to the existing TTL value, if provided then it must be >= 0. A value of 0 is considered 'unset', meaning there is no default.
  • Entry records in the database and API calls

During Entry creation and update

  • If the API call contains a non-zero X509SvidTtl value then that will be stored, else the config default x509SvidTtl value is used
  • If the API call contains a non-zero JWTSvidTtl value then that will stored, else the config default jwtSvidTtl value is used

During X509-SVID creation

  • If the API call contains a non-zero TTL value then that is used, else
  • If the stored record contains a non-zero X509SvidTtl value then that will be used, else
  • If the stored record contains a non-zero TTL value then that will be used,
  • The hard-coded default X509SvidTTL value will be used

During JWT-SVID creation

  • If the API call contains a non-zero TTL value then that is used, else
  • If the stored record contains a non-zero JWTSvidTtl value then that will be used, else
  • If the stored record contains a non-zero TTL value then that will be used,
  • The hard-coded default JWTSvidTTL value will be used

All of Ttl, X509SvidTtl, and JwtSvidTtl will be considered during the following cases

  • All must be valid with-respect-to the configured CA TTL - they are all part of the min/max validation checks
  • Entry sorting now includes each of Ttl, X509SvidTtl, and JwtSvidTtl

Signed-off-by: Dennis Gove dgove1@bloomberg.net

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

@dennisgove
Copy link
Contributor Author

Note: failed compilation should be expected until spiffe/spire-api-sdk#29 is merged and the go.sum references the new version.

pkg/common/util/sort.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@MarcosDY MarcosDY left a comment

Choose a reason for hiding this comment

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

Thanks for this!!! and great work

cmd/spire-server/cli/jwt/mint_test.go Outdated Show resolved Hide resolved
doc/spire_server.md Outdated Show resolved Hide resolved
doc/spire_server.md Outdated Show resolved Hide resolved
doc/spire_server.md Outdated Show resolved Hide resolved
cmd/spire-server/cli/run/run.go Outdated Show resolved Hide resolved
pkg/server/datastore/sqlstore/sqlstore.go Outdated Show resolved Hide resolved
pkg/server/datastore/sqlstore/sqlstore.go Outdated Show resolved Hide resolved
pkg/server/datastore/sqlstore/sqlstore.go Outdated Show resolved Hide resolved
pkg/server/datastore/sqlstore/sqlstore.go Outdated Show resolved Hide resolved
proto/spire/common/common.proto Outdated Show resolved Hide resolved
@dennisgove dennisgove force-pushed the jwt-ttl-2700 branch 2 times, most recently from c075316 to eff1b4c Compare September 25, 2022 16:55
@dennisgove
Copy link
Contributor Author

Thank you, @MarcosDY for your feedback. Every comment with a 👍 has been corrected. The only outstanding issue is the upgrade and downgrade question, for which I've posted my thoughts.

@azdagron azdagron added this to the 1.5.0 milestone Sep 29, 2022
@azdagron
Copy link
Member

Hey, @dennisgove! We talked about this yesterday in our maintainer sync. We talked about a few things and are letting things marinate a bit before making a decision. We should be able to get back to you with direction early next week.

@azdagron
Copy link
Member

azdagron commented Oct 6, 2022

Sorry it took so longer to get back here. I think we are good with the proposal you outlined for upgrade downgrade.

Additionally, as far as the API layer is concerned, we concluded that we think the compat story is a little easier if, instead of adding two new fields, we rename the existing ttl field to x509_svid_ttl and only introduce the new jwt_svid_ttl field. This allows for applications, like the spire-controller-manager, to continue working with SPIRE without change. It also supports the behavior in your proposal wherein ttl is more or less reflected under x509_svid_ttl and vice-versa.

How does that sound?

@dennisgove
Copy link
Contributor Author

rename the existing ttl field to x509_svid_ttl

I have two concerns with this approach. Though please let me know if I'm misunderstanding the intent here.

First, we've already added two new fields in the database for this feature. #3174 added columns x509_svid_ttl and jwt_svid_ttl while keeping column ttl, and was released as part of 1.3.2. That said, this isn't a blocker as both the ttl and x509_svid_ttl columns can be updated using the same API field.

Second, and I think more importantly, is that wouldn't a rename of the API field from ttl to x509SvidTtl be a breaking change for anything that's using the ttl field? CLI calls with -ttl won't work anymore. API calls with requests containing a ttl field would no longer work. Clients expecting a ttl field in a response will break. The introduction of the x509_svid_ttl field was in an effort to avoid that breaking change right away. In effect, have a few versions which support both ttl and x509_svid_ttl so that clients and libraries can migrate to x509_svid_ttl and only after a reasonable period of time would we remove ttl.

That said, I could see value in the following approach, though there are still issues with it.

  1. All internal code renames the ttl field to x509_svid_ttl and there will only be x509_svid_ttl and jwt_svid_ttl fields in all internal types. One problem here is that the database model will still have 3 fields, for now.
  2. All external API and CLI references for ttl will support the alias x509_svid_ttl. They mean the same thing and only one can be set. If a client is sending an API request with ttl then they cannot include x509_svid_ttl, and vice-versa. One problem here is, what field name is returned from the API? We still have the breaking change problem if a client expects the field to be called ttl and it's called x509_svid_ttl.

@azdagron
Copy link
Member

azdagron commented Oct 7, 2022

Second, and I think more importantly, is that wouldn't a rename of the API field from ttl to x509SvidTtl be a breaking change for anything that's using the ttl field?

This would only be a breaking change at compile time, and only after the software updates their dependency on the SDK. Already built software would continue to function just fine, since the protobuf field number would remain the same, i.e., wire compatibility would not break.

Consider the inverse. If we move forward then with it as-is, then software, at some point, needs to cut over to using the new x509_svid_ttl field. The software would no longer work with old versions of SPIRE.
SPIRE itself would also, at some point, presumably want to remove the ttl field. This means that software that has not updated to use x509_svid_ttl would stop functioning with new versions of SPIRE.

@dennisgove
Copy link
Contributor Author

the protobuf field number would remain the same, i.e., wire compatibility would not break.

That is something I forgot about - thanks for clearing that up. This plan sounds good to me. I'll get started on those changes. For my own planning purposes, when are we intending to cut 1.5, so I can be sure to get these in with plenty of time?

@azdagron
Copy link
Member

azdagron commented Oct 7, 2022

Yikes! Milestone wasn't updated. I just updated it with our tentative release date of Nov 2nd. We try and release every four weeks. We usually pick our commit about a week before the release, so that would give you until Oct 26th. We rarely have a hard release deadline and therefore try and be flexible, so if things drag out a little longer, that is ok.

@azdagron
Copy link
Member

azdagron commented Oct 7, 2022

Thanks for picking up this work by the way. Really appreciate the meaty contribution.

@azdagron
Copy link
Member

Hey @dennisgove! Just wanted to follow up and see how things were going here. Do you still feel comfortable hitting the commit pick date for the next release? No pressure. Let us know if you need anything.

@dennisgove
Copy link
Contributor Author

Hi @azdagron. I do still feel comfortable hitting the OCtober 26th date. I had a little issue when I swapped my laptop to an M1 but have gotten that resolved.

dennisgove added a commit to dennisgove/spire-api-sdk that referenced this pull request Oct 20, 2022
Per comment spiffe/spire#3445 (comment) it was decided to
1. Rename the TTL field to X509SvidTtl
2. Add the new JwtSvidTtl field

This change augments commit spiffe@3c5e450

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
azdagron pushed a commit to spiffe/spire-api-sdk that referenced this pull request Oct 20, 2022
Per comment spiffe/spire#3445 (comment) it was decided to
1. Rename the TTL field to X509SvidTtl
2. Add the new JwtSvidTtl field

This change augments commit 3c5e450

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
@dennisgove
Copy link
Contributor Author

@azdagron I believe I've made all the necessary changes, including in the tests. But I'm not 100% that I haven't missed something.

Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Looks like we missed updating go.mod with the latest pseudo-version. CI/CD is also not passing.

cmd/spire-server/cli/entry/util.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/entry/create.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/run/run.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/run/run.go Outdated Show resolved Hide resolved
doc/SPIRE101.md Outdated Show resolved Hide resolved
pkg/agent/manager/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/common/telemetry/names.go Outdated Show resolved Hide resolved
pkg/server/api/entry.go Show resolved Hide resolved
pkg/server/config.go Outdated Show resolved Hide resolved
pkg/server/config.go Outdated Show resolved Hide resolved
@dennisgove
Copy link
Contributor Author

dennisgove commented Oct 24, 2022

Looks like we missed updating go.mod with the latest pseudo-version. CI/CD is also not passing.

@azdagron, is this something I should be changing? If so, what should I change?

@dennisgove dennisgove force-pushed the jwt-ttl-2700 branch 2 times, most recently from cafa109 to ac3b5ad Compare October 24, 2022 03:48
@azdagron
Copy link
Member

azdagron commented Oct 24, 2022

is this something I should be changing?

Yeah, you need to update the spire-api-sdk dependency to pick up the rename change you did that was merged onto the next branch.

$ go get github.com/spiffe/spire-api-sdk@5895a027994460226988e32ac2ba7c963dfca4f7

Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Thanks for the update @dennisgove. Most comments are naming and other minutiae. Also, from the failed CI/CD run, it looks like we missed updating some of the CLI tests that only run for windows.
While that is being fixed, I'll build this and give it a run through to sanity check things.

cmd/spire-server/cli/entry/util.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/entry/util.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/entry/util.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/entry/util.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/run/run.go Outdated Show resolved Hide resolved
doc/spire_server.md Outdated Show resolved Hide resolved
pkg/agent/manager/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/agent/manager/cache/cache_test.go Outdated Show resolved Hide resolved
pkg/server/api/entry.go Show resolved Hide resolved
pkg/server/ca/manager.go Outdated Show resolved Hide resolved
@dennisgove
Copy link
Contributor Author

Thanks @azdagron. I plan to have those items pushed by tonight.

Fixes spiffe#2700

This change adds support for X509 and JWT specific SVID TTLs in each of the following places
 * Default values in spire-server configuration. Similar to the existing TTL value, if provided then it must be >= 0. A value of 0 is considered 'unset', meaning there is no default.
 * Entry records in the database and API

During Entry creation and update
 * If the API call contains a non-zero X509SvidTtl value then that will be stored, else the config default x509SvidTtl value is used
 * If the API call contains a non-zero JWTSvidTtl value then that will stored, else the config default jwtSvidTtl value is used

During X509-SVID creation
 * If the API call contains a non-zero TTL value then that is used, else
 * If the stored record contains a non-zero X509SvidTtl value then that will be used, else
 * If the stored record contains a non-zero TTL value then that will be used,
 * The hard-coded default X509SvidTTL value will be used

During JWT-SVID creation
 * If the API call contains a non-zero TTL value then that is used, else
 * If the stored record contains a non-zero JWTSvidTtl value then that will be used, else
 * If the stored record contains a non-zero TTL value then that will be used,
 * The hard-coded default JWTSvidTTL value will be used

X509SvidTtl and JwtSvidTtl will be considered during the following cases
 * All must be valid with-respect-to the configured CA TTL - they are all part of the min/max validation checks
 * Entry sorting now includes each of X509SvidTtl and JwtSvidTtl

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
…ng ttl

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Excellent, @dennisgove . I feel much better about this. I've left a nitpicky comment, but will start doing some manual testing on this to build confidence. I think we can likely merge this today. Thank you for the great work and significant contribution!

@azdagron
Copy link
Member

I did some manual testing and I think we're good here. Once that final nit is fixed, I think we'll be ready to merge. I can push a commit if needed.

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
@dennisgove
Copy link
Contributor Author

Thank you @azdagron and @MarcosDY for all your feedback and help with this change. I really appreciate it!

@azdagron azdagron merged commit bcc05ff into spiffe:main Oct 26, 2022
azdagron pushed a commit to azdagron/spire-api-sdk that referenced this pull request Oct 28, 2022
Per comment spiffe/spire#3445 (comment) it was decided to
1. Rename the TTL field to X509SvidTtl
2. Add the new JwtSvidTtl field

This change augments commit spiffe@3c5e450

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
azdagron pushed a commit to spiffe/spire-api-sdk that referenced this pull request Oct 28, 2022
Per comment spiffe/spire#3445 (comment) it was decided to
1. Rename the TTL field to X509SvidTtl
2. Add the new JwtSvidTtl field

This change augments commit 3c5e450

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023
…piffe#3445)

* Adds support for X509 and JWT specific SVID TTLs

Fixes spiffe#2700

This change adds support for X509 and JWT specific SVID TTLs in each of the following places
 * Default values in spire-server configuration. Similar to the existing TTL value, if provided then it must be >= 0. A value of 0 is considered 'unset', meaning there is no default.
 * Entry records in the database and API

During Entry creation and update
 * If the API call contains a non-zero X509SvidTtl value then that will be stored, else the config default x509SvidTtl value is used
 * If the API call contains a non-zero JWTSvidTtl value then that will stored, else the config default jwtSvidTtl value is used

During X509-SVID creation
 * If the API call contains a non-zero TTL value then that is used, else
 * If the stored record contains a non-zero X509SvidTtl value then that will be used, else
 * If the stored record contains a non-zero TTL value then that will be used,
 * The hard-coded default X509SvidTTL value will be used

During JWT-SVID creation
 * If the API call contains a non-zero TTL value then that is used, else
 * If the stored record contains a non-zero JWTSvidTtl value then that will be used, else
 * If the stored record contains a non-zero TTL value then that will be used,
 * The hard-coded default JWTSvidTTL value will be used

X509SvidTtl and JwtSvidTtl will be considered during the following cases
 * All must be valid with-respect-to the configured CA TTL - they are all part of the min/max validation checks
 * Entry sorting now includes each of X509SvidTtl and JwtSvidTtl

Signed-off-by: Dennis Gove <dgove1@bloomberg.net>
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.

SPIRE Agent now respects entry's TTL when issuing JWT-SVIDs
3 participants