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 improved secret management #301

Merged
merged 4 commits into from
Oct 12, 2023
Merged

Add improved secret management #301

merged 4 commits into from
Oct 12, 2023

Conversation

macmv
Copy link
Contributor

@macmv macmv commented Oct 11, 2023

Ticket(s): ENG-5635

Adds a Secret helper class, which manages a scoped secret.

This makes it so that fauna shell <scope> will append the given scope to the scope in your project file. The behavior is a little weird:

% fauna shell bar
Connected to endpoint: neil-2-us database: foo/bar
Starting shell for database bar
Type Ctrl+D or .exit to exit the shell
bar>

Note that it says "connected to database foo/bar", and then says "Starting shell for database bar."

Not sure if we want to fix this or not, but I think its fine as-is.

@macmv macmv requested a review from fauna-chase October 11, 2023 01:40
Copy link
Contributor

@fauna-chase fauna-chase left a comment

Choose a reason for hiding this comment

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

overall this looks good to me, my only real question would be if we currently allow users to provide custom roles via a scoped secret and this change would then break them.

src/lib/config/index.ts Outdated Show resolved Hide resolved
src/lib/secret.ts Outdated Show resolved Hide resolved
src/lib/secret.ts Show resolved Hide resolved
src/lib/secret.ts Show resolved Hide resolved
src/lib/fauna-command.js Show resolved Hide resolved
@macmv macmv force-pushed the correct-secret-management branch 2 times, most recently from 930cea0 to 8675352 Compare October 11, 2023 18:06
Copy link
Contributor

@fauna-chase fauna-chase left a comment

Choose a reason for hiding this comment

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

Looking good, just have one more question on backwards compatibility.

Are we doing a major version release for the new shell? If so perhaps we could just drop a note on that specific case and just call it a day.

return new Secret({ key, allowScope: !key.includes(":") });
}

static parse(key: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does the 'current' shell allow you to have endpoints in your file that are scoped?

[endpoint.child]
secret=secret:child:admin
secret=secret:child:@role/myrole

maybe we don't care? just trying to think through if this could lead to backwards incompatible changes for users and where we draw the line on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the current shell lets you do that, but it will break if you set a database path in fauna eval or fauna shell. It seems like something we ought to just disallow.

test/commands/eval.test.js Outdated Show resolved Hide resolved
@macmv macmv force-pushed the correct-secret-management branch from 77b1c44 to 0fd9144 Compare October 11, 2023 19:09
@macmv macmv force-pushed the correct-secret-management branch from 0fd9144 to 4c6d798 Compare October 12, 2023 18:47
@macmv macmv changed the base branch from main to add-secret-helper October 12, 2023 18:48
Base automatically changed from add-secret-helper to main October 12, 2023 19:20
@macmv macmv merged commit ceffdc5 into main Oct 12, 2023
@macmv macmv deleted the correct-secret-management branch October 12, 2023 19:58
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.

2 participants