-
Notifications
You must be signed in to change notification settings - Fork 881
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
Conversation
This enables zero-trust authentication for Azure instances. Now we support the three major clouds: AWS, Azure, and GCP 😎.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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" { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
This enables zero-trust authentication for Azure instances. Now
we support the three major clouds: AWS, Azure, and GCP 😎.