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.

Sign up for GitHub

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

Jump to bottom

bug(vault): Correctly handle credential stores with expired tokens #2399

Merged
merged 5 commits into from
Sep 6, 2022
Merged

bug(vault): Correctly handle credential stores with expired tokens #2399

merged 5 commits into from
Sep 6, 2022

Conversation

louisruch
Copy link
Collaborator

@louisruch louisruch commented Aug 27, 2022 •
edited
Loading

Fixes #2349 and fixes #2179

Results from some manual testing:

Read expired store:

$ boundary credential-stores read -id csvlt_avrjIaDzMb

Credential Store information:
  Created Time:        Fri, 26 Aug 2022 20:57:10 PDT
  ID:                  csvlt_avrjIaDzMb
  Type:                vault
  Updated Time:        Fri, 26 Aug 2022 20:58:09 PDT
  Version:             2

  Scope:
    ID:                p_1234567890
    Name:              Generated project scope
    Parent Scope ID:   o_1234567890
    Type:              project

  Authorized Actions:
    no-op
    read
    update
    delete

  Authorized Actions on Credential Store's Collections:
    credential-libraries:
      create
      list

  Attributes:
    Address:           http://127.0.0.1:8200
    Token HMAC:        tYIknumwvLrphPysRpyqrJIe-AxegPRNQJKkWikkq88
    Token Status:      expired

Update expired store:

$ boundary credential-stores update vault -id csvlt_avrjIaDzMb -vault-token s.vtSS09vFDJ8qmQOEn1PLcaTs

Credential Store information:
  Created Time:        Fri, 26 Aug 2022 20:57:10 PDT
  ID:                  csvlt_avrjIaDzMb
  Type:                vault
  Updated Time:        Fri, 26 Aug 2022 20:59:51 PDT
  Version:             3

  Scope:
    ID:                p_1234567890
    Name:              Generated project scope
    Parent Scope ID:   o_1234567890
    Type:              project

  Authorized Actions:
    no-op
    read
    update
    delete

  Authorized Actions on Credential Store's Collections:
    credential-libraries:
      create
      list

  Attributes:
    Address:           http://127.0.0.1:8200
    Token HMAC:        43ET8x7x9rlxUVUul80pBvgNj4lSaLOLsAAVN87I7_g
    Token Status:      current

Read after update:

$ boundary credential-stores read -id csvlt_avrjIaDzMb

Credential Store information:
  Created Time:        Fri, 26 Aug 2022 20:57:10 PDT
  ID:                  csvlt_avrjIaDzMb
  Type:                vault
  Updated Time:        Fri, 26 Aug 2022 20:59:51 PDT
  Version:             3

  Scope:
    ID:                p_1234567890
    Name:              Generated project scope
    Parent Scope ID:   o_1234567890
    Type:              project

  Authorized Actions:
    no-op
    read
    update
    delete

  Authorized Actions on Credential Store's Collections:
    credential-libraries:
      create
      list

  Attributes:
    Address:           http://127.0.0.1:8200
    Token HMAC:        43ET8x7x9rlxUVUul80pBvgNj4lSaLOLsAAVN87I7_g
    Token Status:      current

@louisruch louisruch force-pushed the louis-vault branch 3 times, most recently from d1bbd4c to 9ec7ec1 Compare August 27, 2022 04:28
@louisruch louisruch requested review from ddebko and mgaffney August 27, 2022 18:38
@louisruch louisruch added this to the 0.10.3 milestone Aug 27, 2022
@louisruch louisruch requested a review from tmessi August 27, 2022 20:06
@louisruch
bug(vault): Correctly handle credential stores with expired tokens
191a854 
Boundary makes use of a database view to perform CRUD operations on
Vault credential stores and libraries. This view did not include
credential stores with tokens that have expired in Vault, causing
errors when attempting to perform any action against them.
@louisruch
Copy link
Collaborator Author

louisruch commented Sep 1, 2022 •
edited
Loading

@mgaffney PTAL, please note I did rebase on main and force pushed. The original implementation is completely different so no need to see what changed since the last time you looked. But if you pull locally you will have a conflict.
Also happy to discuss naming of views and structs
@ddebko @tmessi If you have the time PTAL

ddebko
ddebko approved these changes Sep 1, 2022
View reviewed changes
Copy link
Collaborator

@ddebko ddebko left a comment

Choose a reason for hiding this comment

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

LGTM

mgaffney
mgaffney reviewed Sep 1, 2022
View reviewed changes
internal/credential/vault/private_store.go Outdated Show resolved Hide resolved
internal/db/schema/migrations/oss/postgres/48/01_vault_credentials.up.sql Outdated Show resolved Hide resolved
internal/db/schema/migrations/oss/postgres/48/01_vault_credentials.up.sql Outdated Show resolved Hide resolved
internal/db/schema/migrations/oss/postgres/48/01_vault_credentials.up.sql Outdated
cert.certificate_key as ct_client_key, -- encrypted
cert.key_id as client_key_id
from credential_vault_store store
left join credential_vault_token token
Copy link
Member

Choose a reason for hiding this comment

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

Why does this view need to return a value when there is no current vault token?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This view is used in the update path of the store, so it needs to return the store to update even if the token is expired.
It is also reused in the credential_vault_library_issue_credentials view, when issuing credentials we return an error if the any of the requested libraries have an expired token. We could modify this to do a len compare and validate the number of requests and returned libraries are equal, but the expired error is probably more clear.

internal/db/schema/migrations/oss/postgres/48/01_vault_credentials.up.sql Outdated Show resolved Hide resolved
@vercel
Copy link

vercel bot commented Sep 1, 2022

Deployment failed with the following error:

Invalid website/vercel.json file provided

@louisruch louisruch requested a review from mgaffney September 1, 2022 17:52
@jefferai
Copy link
Member

jefferai commented Sep 2, 2022

@louisruch Note that you'll need to renumber the migration

mgaffney
mgaffney approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@mgaffney mgaffney left a comment

Choose a reason for hiding this comment

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

Nice work!

@louisruch louisruch merged commit d7c4c64 into main Sep 6, 2022
@louisruch louisruch deleted the louis-vault branch September 6, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jefferai jefferai jefferai left review comments

@ddebko ddebko ddebko approved these changes

@mgaffney mgaffney mgaffney approved these changes

@tmessi tmessi Awaiting requested review from tmessi

Assignees
No one assigned
Labels
api core/daemon core/db core/proto core/sql core
Projects
None yet
Milestone
0.10.4
Development

Successfully merging this pull request may close these issues.

database migration failed credential-stores disappear
4 participants
@louisruch @jefferai @mgaffney @ddebko