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

Vault Support (GSI-973) #8

Merged
merged 15 commits into from
Oct 30, 2024
Merged

Conversation

TheByronHimes
Copy link
Member

@TheByronHimes TheByronHimes commented Oct 28, 2024

GET /secrets/{vault_path}: returns a list of all keys in the specified vault

DELETE /secrets/{vault_path}: deletes all keys in the specified vault

@coveralls
Copy link

coveralls commented Oct 28, 2024

Pull Request Test Coverage Report for Build 11593029394

Details

  • 66 of 70 (94.29%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 92.73%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/sms/adapters/inbound/fastapi_/routers/secrets.py 15 19 78.95%
Totals Coverage Status
Change from base Build 10704242679: 0.2%
Covered Lines: 625
Relevant Lines: 674

💛 - Coveralls

@TheByronHimes TheByronHimes marked this pull request as ready for review October 28, 2024 16:00
@dontseyit
Copy link
Member

@TheByronHimes

Given that only the case of EKSS exists at the moment, the current SMS configuration is enough but also limited to only one SMS per path in the vault. It would be more convenient not to have only one vault_path configured but passed in the request (e.g. GET /secrets/ekss) so one SMS instance can operate on multiple paths.

Security vise it should be safe because the policies attached to the token grant the necessary permissions for those paths. It may require catching the case of permission error in the code to return an appropriate response to the requester.

Copy link
Member

@mephenor mephenor left a comment

Choose a reason for hiding this comment

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

In addition to what was already said, there's one small doc inconsistency

secrets_handler: dummies.SecretsHandlerPortDummy,
_token: Annotated[TokenAuthContext, require_token],
):
"""Delete one or more secrets from the vault"""
Copy link
Member

Choose a reason for hiding this comment

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

It's deleting all of them, though

Copy link
Member Author

Choose a reason for hiding this comment

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

Agh, outdated docstring. Thought I got all of that. Thanks

Copy link
Member

@dontseyit dontseyit left a comment

Choose a reason for hiding this comment

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

I say: it looks good, thank you.

"""Delete all secrets stored in the vault."""
self.secrets = []
self.secrets.pop(vault_path, None)
Copy link
Member

Choose a reason for hiding this comment

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

This would also remove the key from the dict.
Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the whole dummy class and replaced it with Mock in 536198b

AsyncTestClient(app=app) as client,
):
response = await client.get("/secrets/doesnotexist", headers=HEADERS)
assert response.status_code == 200
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a 200?

Copy link
Member

@dontseyit dontseyit Oct 30, 2024

Choose a reason for hiding this comment

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

I think because you cannot distinguish the invalid path from an empty path, it returns an empty list in either case, and I think it should.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regardless of if you return an empty list or raise an error, you can't tell if it's a path problem or an empty vault.
But a non-existent vault has no secrets, so I don't really see a problem with doing it that way.

@TheByronHimes TheByronHimes merged commit e1d98e8 into main Oct 30, 2024
9 checks passed
@TheByronHimes TheByronHimes deleted the feature/vault_management_GSI-973 branch October 30, 2024 12:41
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.

4 participants