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

feat: Add AWS instance identity authentication #570

Merged
merged 5 commits into from
Mar 28, 2022
Merged

feat: Add AWS instance identity authentication #570

merged 5 commits into from
Mar 28, 2022

Conversation

kylecarbs
Copy link
Member

This allows zero-trust authentication for all AWS instances.

Prior to this, AWS instances could be used by passing CODER_TOKEN
as an environment variable to the startup script. AWS explicitly
states that secrets should not be passed in startup scripts because
it's user-readable.

@kylecarbs kylecarbs requested review from Emyrk and coadler March 26, 2022 01:56
@kylecarbs kylecarbs self-assigned this Mar 26, 2022
This allows zero-trust authentication for all AWS instances.

Prior to this, AWS instances could be used by passing `CODER_TOKEN`
as an environment variable to the startup script. AWS explicitly
states that secrets should not be passed in startup scripts because
it's user-readable.
@kylecarbs
Copy link
Member Author

@bpmct once this lands, we can remove CODER_TOKEN from examples in favor of the instance_id approach. Much more secure!

@codecov
Copy link

codecov bot commented Mar 26, 2022

Codecov Report

Merging #570 (eec8e29) into main (be8389f) will increase coverage by 0.30%.
The diff coverage is 70.19%.

@@            Coverage Diff             @@
##             main     #570      +/-   ##
==========================================
+ Coverage   63.43%   63.73%   +0.30%     
==========================================
  Files         195      196       +1     
  Lines       11321    11514     +193     
  Branches       85       85              
==========================================
+ Hits         7181     7338     +157     
- Misses       3380     3399      +19     
- Partials      760      777      +17     
Flag Coverage Δ
unittest-go- 62.91% <70.19%> (+0.28%) ⬆️
unittest-go-macos-latest 58.47% <70.19%> (+0.47%) ⬆️
unittest-go-ubuntu-latest 61.42% <70.19%> (+0.43%) ⬆️
unittest-go-windows-2022 57.59% <70.19%> (+0.21%) ⬆️
unittest-js 63.32% <ø> (ø)
Impacted Files Coverage Δ
codersdk/workspaceresourceauth.go 47.50% <41.07%> (-15.00%) ⬇️
coderd/workspaceresourceauth.go 37.73% <44.44%> (+0.37%) ⬆️
coderd/awsidentity/awsidentity.go 67.44% <67.44%> (ø)
cli/workspaceagent.go 77.77% <90.62%> (+11.11%) ⬆️
coderd/coderdtest/coderdtest.go 98.63% <96.49%> (-0.53%) ⬇️
cli/cliui/prompt.go 78.26% <100.00%> (ø)
coderd/coderd.go 96.45% <100.00%> (+0.02%) ⬆️
provisioner/echo/serve.go 54.40% <0.00%> (-2.40%) ⬇️
coderd/workspaceresources.go 59.72% <0.00%> (-1.81%) ⬇️
provisionerd/provisionerd.go 80.02% <0.00%> (-0.15%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be8389f...eec8e29. Read the comment docs.

Comment on lines +18 to +30
const (
Other Region = "other"
HongKong Region = "hongkong"
Bahrain Region = "bahrain"
CapeTown Region = "capetown"
Milan Region = "milan"
China Region = "china"
GovCloud Region = "govcloud"
)

var (
All = []Region{Other, HongKong, Bahrain, CapeTown, Milan, China, GovCloud}
)
Copy link
Member

Choose a reason for hiding this comment

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

These variable names should probably be prefixed with Region.

Copy link
Member Author

Choose a reason for hiding this comment

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

It felt unnecessarily verbose due to the small scope of the package.

coderd/awsidentity/awsidentity.go Outdated Show resolved Hide resolved
coderd/coderdtest/coderdtest.go Outdated Show resolved Hide resolved
codersdk/workspaceresourceauth.go Show resolved Hide resolved
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

I posted some nits. Nothing that will block a merge.

I am missing context on what this is actually used for though. It's only used for testing? We test in AWS and GCP?

cli/workspaceagent_test.go Show resolved Hide resolved
coderd/coderdtest/coderdtest.go Show resolved Hide resolved
coderd/coderdtest/coderdtest.go Show resolved Hide resolved
coderd/coderdtest/coderdtest_test.go Show resolved Hide resolved
coderd/workspaceresourceauth_test.go Show resolved Hide resolved
codersdk/workspaceresourceauth.go Show resolved Hide resolved
@kylecarbs kylecarbs enabled auto-merge (squash) March 28, 2022 19:16
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.

None yet

3 participants