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

Workload ID: Add Auth Server JWT SVID support #46968

Merged
merged 8 commits into from
Oct 2, 2024

Conversation

strideynet
Copy link
Contributor

@strideynet strideynet commented Sep 30, 2024

Adds the appropriate RPCs to the Auth Server to support the issuance of JWT SPIFFE SVIDs.

The next PR will add support for these within tbot.

Part of #38930

@strideynet strideynet added backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry labels Sep 30, 2024
lib/jwt/jwt.go Outdated Show resolved Hide resolved
}

// Create a JTI to uniquely identify this JWT for audit logging purposes
jti, err = utils.CryptoRandomHex(jtiLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the jti recommended to be just a random hex string rather than something like a UUID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as i'm aware within SPIFFE, there's no real requirement for it to take any specific form. I'd be open to switching it to a UUID if there's some advantage I'm not seeing.

Copy link
Contributor

Choose a reason for hiding this comment

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

It prompted me to check the JWT spec and this is what I found in the RFC 7519, pasting for reference:

The "jti" (JWT ID) claim provides a unique identifier for the JWT.
The identifier value MUST be assigned in a manner that ensures that
there is a negligible probability that the same value will be
accidentally assigned to a different data object; if the application
uses multiple issuers, collisions MUST be prevented among values
produced by different issuers as well. The "jti" claim can be used
to prevent the JWT from being replayed

Copy link
Contributor

@flyinghermit flyinghermit left a comment

Choose a reason for hiding this comment

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

lgtm

api/proto/teleport/legacy/types/events/events.proto Outdated Show resolved Hide resolved
@@ -213,7 +227,7 @@ func (wis *WorkloadIdentityService) signX509SVID(
// Setup audit log event, we will emit these even on failure to catch any
// authz denials
var serialNumber *big.Int
var spiffeID *url.URL
var spiffeID spiffeid.ID
Copy link
Contributor

Choose a reason for hiding this comment

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

it took me a while to build mental modal on spiffeID vs. spiffeid. Maybe if we can alias the imported one as gospiffe?

}

// Create a JTI to uniquely identify this JWT for audit logging purposes
jti, err = utils.CryptoRandomHex(jtiLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

It prompted me to check the JWT spec and this is what I found in the RFC 7519, pasting for reference:

The "jti" (JWT ID) claim provides a unique identifier for the JWT.
The identifier value MUST be assigned in a manner that ensures that
there is a negligible probability that the same value will be
accidentally assigned to a different data object; if the application
uses multiple issuers, collisions MUST be prevented among values
produced by different issuers as well. The "jti" claim can be used
to prevent the JWT from being replayed

Co-authored-by: Sakshyam Shah <sshah@goteleport.com>
@strideynet strideynet added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@strideynet strideynet added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@strideynet strideynet added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@strideynet strideynet added this pull request to the merge queue Oct 2, 2024
Merged via the queue into master with commit d8f68d7 Oct 2, 2024
42 checks passed
@strideynet strideynet deleted the strideynet/jwt-svid-auth-server branch October 2, 2024 10:17
@public-teleport-github-review-bot

@strideynet See the table below for backport results.

Branch Result
branch/v16 Failed

strideynet added a commit that referenced this pull request Oct 2, 2024
* Add new event fields

* Add gRPC API for requesting JWT SVIDs

* Add JWT helper for creating JWT SVID

* Add WorkloadIdentityService implementation for issuing JWT svids

* Fix spelling of compatibility

* Use single time to calculate both expiry and iat

* Update api/proto/teleport/legacy/types/events/events.proto

Co-authored-by: Sakshyam Shah <sshah@goteleport.com>

* Regenerate protos

---------

Co-authored-by: Sakshyam Shah <sshah@goteleport.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 7, 2024
* Workload ID: Add Auth Server JWT SVID support (#46968)

* Add new event fields

* Add gRPC API for requesting JWT SVIDs

* Add JWT helper for creating JWT SVID

* Add WorkloadIdentityService implementation for issuing JWT svids

* Fix spelling of compatibility

* Use single time to calculate both expiry and iat

* Update api/proto/teleport/legacy/types/events/events.proto

Co-authored-by: Sakshyam Shah <sshah@goteleport.com>

* Regenerate protos

---------

Co-authored-by: Sakshyam Shah <sshah@goteleport.com>

* Fix backport

* Fix tests

---------

Co-authored-by: Sakshyam Shah <sshah@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants