Add token list/delete endpoints by avatus · Pull Request #42402 · gravitational/teleport

avatus · 2024-06-04T19:07:53Z

This PR implements list/delete for tokens in the api the same way as
tctl interacts with them. This does not include pagination yet but is
currently being used while the feature is in development. Pagination can
come in the future in the form of a sort cache, but the endpoints won't
change

zmb3 · 2024-06-04T20:35:39Z

+	// Id is the name of the token
+	Id string `json:"id"`


Suggested change

// Id is the name of the token

Id string `json:"id"`

// ID is the name of the token

ID string `json:"id"`

zmb3 · 2024-06-04T20:35:54Z

+	// SafeName returns the name of the token, sanitized appropriately for
+	// join methods where the name is secret.
+	SafeName string `json:"safeName"`
+	// Expiry is the time that the token resource expires


I'd mention what value to expect for tokens which do not expire.

zmb3 · 2024-06-04T20:36:41Z

+
+func MakeJoinToken(token types.ProvisionToken) JoinToken {
+	labels := token.GetSuggestedLabels()
+	suggestedLabels := make([]Label, len(labels))


Suggested change

suggestedLabels := make([]Label, len(labels))

suggestedLabels := make([]Label, 0, len(labels))

Otherwise suggestedLabels is going to be twice as long as you expect.

zmb3 · 2024-06-04T20:38:10Z

+	}, nil
+}
+
+func (h *Handler) deleteToken(_ http.ResponseWriter, r *http.Request, params httprouter.Params, ctx *SessionContext) (interface{}, error) {


I noticed you _'d the ResponseWriter here, but not in getTokens. I would be consistent.

I'm going to put the w back in, as that is consistent with the rest of the api. I used the _ because my linter was throwing a warning but now it isn't? weird. oh well!

zmb3 · 2024-06-04T20:38:24Z

+	expiry time.Time
+}
+
+func Test_getTokens(t *testing.T) {


Suggested change

func Test_getTokens(t *testing.T) {

func TestGetTokens(t *testing.T) {

zmb3 · 2024-06-04T20:38:33Z

+	ctx := context.Background()
+	expiry := time.Now().UTC().Add(30 * time.Minute)
+
+	staticUiToken := ui.JoinToken{


Suggested change

staticUiToken := ui.JoinToken{

staticUIToken := ui.JoinToken{

marcoandredinis · 2024-06-05T07:38:50Z

+	err = clt.DeleteToken(r.Context(), token)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}


💅

Suggested change

err = clt.DeleteToken(r.Context(), token)

if err != nil {

return nil, trace.Wrap(err)

}

if err := clt.DeleteToken(r.Context(), token); err != nil {

return nil, trace.Wrap(err)

}

marcoandredinis · 2024-06-05T07:44:59Z

+		SafeName:        token.GetSafeName(),
+		Expiry:          token.Expiry(),
+		Roles:           token.GetRoles(),
+		IsStatic:        token.Expiry().Equal(time.Unix(0, 0).UTC()),


I'm not sure about this one.
A static token is a token that is configured directly into the teleport.yaml in the auth service. Those exist inside the type types.StaticTokensV2

Which is different from a token with no expiration, which you can create dynamically.
So while static tokens don't have expiration, not all Expiry == 0 are static tokens.

Static tokens are set from the config with zero Unix time (1970-01-01), but if I manually configure a token with no ttl, it's actually set to zero time (0001-01-01) so it's not an ideal distinction but it does work.

Ideally I wanted to use the Origin but it seems the origin for all tokens (in my testing) just return empty string.

TIL, thank you.
Can we convert this check into a token's method? I think this distinction should be really close to the token's definition.

Do you mean something like making JoinToken.IsStaic() ?

avatus · 2024-06-05T20:26:33Z

I've added this method to replace ProvisionTokensFromV1 and all instances that I could find using the old method had to deal with static tokens. I could have just thrown in this SetOrigin there but I was worried it would be used for something that isnt a static token. If that isn't the case, I'll remove this new ProvisionTokensFromStatic method and throw it into the old one. Curious your thoughts

I think that's fine, but it would be better if we changed it at its creation instead of when converting to something else.
So, either on their CheckAndSetDefaults or when parsing the token from the file.

teleport/api/types/statictokens.go

Line 41 in 67a3d64

if err := st.CheckAndSetDefaults(); err != nil {

teleport/lib/config/fileconf.go

Line 950 in 67a3d64

tokens, err := st.Parse()

avatus · 2024-06-05T20:26:55Z

Ok, I added the IsStatic method but I am not relying on the time anymore (it seemed fickle to me from the start). When we fetch static tokens, we create a new ProvisionToken from the available static tokens. I've just set their origin to OriginConfiguration instead of being nil and now IsStatic just checks if Origin() == OriginConfiguration. It seems to be more correct and resilient this way. Let me know what you think

smallinsky · 2024-06-06T09:35:44Z

nit:
what would you say about ConvertToJoinToken ?

all the other "make ui struct" methods for the other UI resources follow the MakeX pattern so I'll keep it to stay consistent.

smallinsky · 2024-06-06T09:35:51Z

Suggested change

func MakeJoinTokens(tokens []types.ProvisionToken) (joinTokens []JoinToken) {

func ConvertToJoinTokens(tokens []types.ProvisionToken) (joinTokens []JoinToken) {

smallinsky · 2024-06-06T09:36:03Z

nit: missing docs.

smallinsky · 2024-06-06T09:58:53Z

This is a bit breaking way to return labels where labels are joined by space. What if a label contains a space like m = {"zone: []string{"district 1", "district 2"}} ?

Can we reuse ui.makeLabels

We've decided to remove the suggested labels all together from the UI table, so i'll just remove this code. We will be sending the full json value of the token as well for a "Details" page where this information will be available, but we don't need it in the UI struct individually

smallinsky · 2024-06-06T10:10:15Z

Can we hide the :id in the DELETE request body instead of exposing it in URL ?

Let say that a user has permission to list tokens but it don't have permission to delete tokens. And a customer infrastructure tools like:

LB -> Ingress -> Teleport Proxy

Please note that :id == token_secret in case of static tokens.

When a client calls /webapi/tokens/token_secret with missing permission to delete secret the Ingress can log the request HTTP Path and expose token_secret in Ingress logs.

Great idea, i'll make the change! typically DELETE requests don't have a body and I don't think our client supports it but I'll see how it works

Decided to send via a custom header as some servers/load balancers might just ignore any request body sent with a DELETE

zmb3 · 2024-06-07T14:26:39Z

Should this be something like X-Teleport-TokenName?

yes it should

This PR implements list/delete for tokens in the api the same way as tctl interacts with them. This does not include pagination yet but is currently being used while the feature is in development. Pagination can come in the future in the form of a sort cache, but the endpoints won't change

* Add token list/delete endpoints (#42402) This PR implements list/delete for tokens in the api the same way as tctl interacts with them. This does not include pagination yet but is currently being used while the feature is in development. Pagination can come in the future in the form of a sort cache, but the endpoints won't change * Add token ACL/routes * Add initial join tokens UI * Add Allow and GCP fields to ui join token * Fix missing styled components * Fix empty join token state (#45413) Empty join token responses would show an error instead of an empty list message * Update button case

avatus added the no-changelog Indicates that a PR does not require a changelog entry label Jun 4, 2024

github-actions Bot requested review from EdwardDowling and smallinsky June 4, 2024 19:08

github-actions Bot added the size/md label Jun 4, 2024

avatus force-pushed the avatus/api_tokens branch from 947dc38 to d44bfb3 Compare June 4, 2024 19:29

zmb3 approved these changes Jun 4, 2024

View reviewed changes

marcoandredinis reviewed Jun 5, 2024

View reviewed changes

avatus commented Jun 5, 2024

View reviewed changes

smallinsky reviewed Jun 6, 2024

View reviewed changes

avatus force-pushed the avatus/api_tokens branch from 56dda05 to 24d7fc1 Compare June 6, 2024 22:06

smallinsky approved these changes Jun 7, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from EdwardDowling June 7, 2024 07:28

zmb3 reviewed Jun 7, 2024

View reviewed changes

marcoandredinis approved these changes Jun 7, 2024

View reviewed changes

avatus force-pushed the avatus/api_tokens branch from 88363ca to edb9a20 Compare June 7, 2024 17:24

avatus force-pushed the avatus/api_tokens branch from 4b5f250 to ffc7071 Compare June 7, 2024 22:40

avatus enabled auto-merge June 7, 2024 22:45

avatus added this pull request to the merge queue Jun 7, 2024

Merged via the queue into master with commit 1a271a4 Jun 7, 2024

avatus deleted the avatus/api_tokens branch June 7, 2024 23:17

avatus mentioned this pull request Jul 26, 2024

[16] Join Tokens UI #44701

Merged

avatus mentioned this pull request Apr 29, 2025

feat(join-tokens): [44794] API changes for managing GitHub Join Tokens #54374

Merged

	suggestedLabels := make([]Label, len(labels))
	suggestedLabels := make([]Label, 0, len(labels))

	func Test_getTokens(t *testing.T) {
	func TestGetTokens(t *testing.T) {

	staticUiToken := ui.JoinToken{
	staticUIToken := ui.JoinToken{

	func MakeJoinTokens(tokens []types.ProvisionToken) (joinTokens []JoinToken) {
	func ConvertToJoinTokens(tokens []types.ProvisionToken) (joinTokens []JoinToken) {

Conversation

avatus commented Jun 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallinsky Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

avatus Jun 5, 2024 •

edited

Loading

smallinsky Jun 6, 2024 •

edited

Loading

avatus Jun 6, 2024 •

edited

Loading