Skip to content

feat: Add Azure instance identitity authentication #1064

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

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Conversation

kylecarbs
Copy link
Member

This enables zero-trust authentication for Azure instances. Now
we support the three major clouds: AWS, Azure, and GCP 😎.

This enables zero-trust authentication for Azure instances. Now
we support the three major clouds: AWS, Azure, and GCP 😎.
@kylecarbs kylecarbs requested review from johnstcn and coadler April 19, 2022 02:25
@kylecarbs kylecarbs self-assigned this Apr 19, 2022
@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #1064 (64ccf8e) into main (118a47e) will increase coverage by 0.08%.
The diff coverage is 64.10%.

@@            Coverage Diff             @@
##             main    #1064      +/-   ##
==========================================
+ Coverage   66.59%   66.67%   +0.08%     
==========================================
  Files         261      262       +1     
  Lines       15502    15653     +151     
  Branches      152      152              
==========================================
+ Hits        10323    10437     +114     
- Misses       4127     4150      +23     
- Partials     1052     1066      +14     
Flag Coverage Δ
unittest-go-macos-latest 53.46% <64.10%> (+0.20%) ⬆️
unittest-go-postgres- 65.88% <64.10%> (+<0.01%) ⬆️
unittest-go-ubuntu-latest 55.90% <64.10%> (+0.03%) ⬆️
unittest-go-windows-2022 52.90% <64.10%> (+0.11%) ⬆️
unittest-js 67.96% <ø> (ø)
Impacted Files Coverage Δ
coderd/workspaceresourceauth.go 36.44% <30.76%> (-0.71%) ⬇️
coderd/azureidentity/azureidentity.go 34.78% <34.78%> (ø)
codersdk/workspaceagents.go 52.14% <44.44%> (-0.91%) ⬇️
coderd/coderdtest/coderdtest.go 98.87% <96.49%> (-0.47%) ⬇️
cli/agent.go 82.17% <100.00%> (+4.40%) ⬆️
coderd/coderd.go 97.31% <100.00%> (+0.01%) ⬆️
cli/cliui/agent.go 77.19% <0.00%> (-5.27%) ⬇️
coderd/provisionerdaemons.go 59.34% <0.00%> (-0.52%) ⬇️
provisionerd/provisionerd.go 80.88% <0.00%> (-0.30%) ⬇️
... and 5 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 118a47e...64ccf8e. Read the comment docs.

Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Nice! My only suggestion is to swap out fullsailor/pkcs7 for Mozilla's apparently more maintained fork.

@@ -24,6 +25,7 @@ import (
"time"

"cloud.google.com/go/compute/metadata"
"github.com/fullsailor/pkcs7"
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using github.com/mozilla-services/pkcs7 as it appears to be more up-to-date.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

workspace := coderdtest.CreateWorkspace(t, client, codersdk.Me, template.ID)
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)

cmd, _ := clitest.New(t, "agent", "--auth", "azure-instance-identity", "--url", client.URL.String())
Copy link
Member

Choose a reason for hiding this comment

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

😎

}, &http.Client{
Transport: roundTripper(func(r *http.Request) (*http.Response, error) {
// Only handle metadata server requests.
if r.URL.Host != "169.254.169.254" {
Copy link
Member

Choose a reason for hiding this comment

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

👍

Header: make(http.Header),
}, nil
default:
panic("unhandled route: " + r.URL.Path)
Copy link
Member

Choose a reason for hiding this comment

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

(comment): I'm always divided on panicking in test code versus just calling t.Fatalf with the error. I guess panic is way more explicit and should never happen in this case :-)

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'm honestly fine with either. If you have a preference, I'm happy to take it.

@kylecarbs kylecarbs requested a review from a team as a code owner April 19, 2022 13:40
@kylecarbs kylecarbs enabled auto-merge (squash) April 19, 2022 13:41
@kylecarbs kylecarbs merged commit c8246e3 into main Apr 19, 2022
@kylecarbs kylecarbs deleted the azureauth branch April 19, 2022 13:48
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