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 TTL to STS #7605

Merged
merged 6 commits into from
Apr 3, 2024
Merged

Add TTL to STS #7605

merged 6 commits into from
Apr 3, 2024

Conversation

guy-har
Copy link
Contributor

@guy-har guy-har commented Mar 27, 2024

Description

This PR includes:

  • Add TTL to STS
  • Small fixes for the lakeFS STS implementation found along the way
  • Revert the unnecessary change to GenerateJWTLogin login done in the previous STS PR

Copy link

github-actions bot commented Mar 27, 2024

♻️ PR Preview 2421a3f has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

10 passed

@guy-har guy-har changed the title Add TTL to STS Add TTL to STS and implement call from python-wrapper Apr 2, 2024
@guy-har guy-har linked an issue Apr 2, 2024 that may be closed by this pull request
@guy-har guy-har added the include-changelog PR description should be included in next release changelog label Apr 2, 2024
@guy-har guy-har changed the title Add TTL to STS and implement call from python-wrapper Implement Short lived tokens support in python-wrapper and add TTL to STS request Apr 2, 2024
@guy-har guy-har marked this pull request as ready for review April 2, 2024 08:33
@guy-har guy-har changed the title Implement Short lived tokens support in python-wrapper and add TTL to STS request Add TTL to STS Apr 2, 2024
Copy link
Contributor

@idanovo idanovo left a comment

Choose a reason for hiding this comment

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

Looks good
few minor comments

type: integer
format: int64
description: |
The time-to-live for the generated token in seconds. The maximum
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the second sentence here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

@@ -1818,7 +1824,7 @@ paths:
post:
tags:
- experimental
operationId: STSLogin
operationId: stsLogin # change to stsLogin
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@@ -536,7 +539,7 @@ func (c *Controller) Login(w http.ResponseWriter, r *http.Request, body apigen.L
expires := loginTime.Add(duration)
secret := c.Auth.SecretStore().SharedSecret()

tokenString, err := GenerateJWTLogin(secret, user.Username, loginTime.Unix(), expires.Unix())
Copy link
Contributor

Choose a reason for hiding this comment

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

now you're changing it back? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry 🙏

Comment on lines +1069 to +1070
The time-to-live for the generated token in seconds. The maximum
value is 3600 seconds (1 hour) max is 12 hours.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The time-to-live for the generated token in seconds. The maximum
value is 3600 seconds (1 hour) max is 12 hours.
The time-to-live for the generated token in seconds.
The default value is 3600 seconds (1 hour) maximum time allowed is 12 hours.

@guy-har guy-har requested a review from idanovo April 2, 2024 11:24
Copy link
Contributor

@idanovo idanovo left a comment

Choose a reason for hiding this comment

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

LGTM

@guy-har guy-har merged commit a81657f into master Apr 3, 2024
37 checks passed
@guy-har guy-har deleted the chore/add-ttl-to-sts branch April 3, 2024 07:20
emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate Support for Short-Lived Tokens (STS) in Remote Authentication
2 participants