-
Notifications
You must be signed in to change notification settings - Fork 4
Add aws auth support #10
base: master
Are you sure you want to change the base?
Conversation
4killo
commented
May 1, 2022
- with this change, application uses this sdk and run in aws environment can use meta datas for auth.
- with this chnage, appliaction uses this sdk and run in aws enviroment can use metadatas for auth
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've added a bunch of small comments. Nothing huge. In general PR looks good to me
return "", "", err | ||
} | ||
|
||
body, err := ioutil.ReadAll(r.HTTPRequest.Body) |
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.
It is recommended to use io.ReadAll()
in a new code. Please change to io.ReadAll(r.HTTPRequest.Body)
|
||
func (a *authorization) getCallerIdentityRequest() *request.Request { | ||
r, _ := sts.New(a.sess).GetCallerIdentityRequest(nil) | ||
r.Sign() |
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.
Sign()
method can return error. Is it ok to ignore it?
) | ||
|
||
const ( | ||
defaultTLD string = "com" | ||
defaultURLTemplate string = "https://%s.secretsvaultcloud.%s/v1/%s%s" | ||
defaultURLTemplate string = "https://%s.devbambe.%s/v1/%s%s" |
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.
devbambe is internal. Please fix
log.Fatalf("failed to configure vault: %v", err) | ||
} | ||
|
||
secret, err := dsv.Secret("resources:us-east-5:server1") |
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.
in the example/client/client_auth.go file you made this change:
- secret, err := dsv.Secret("path:of:the:secret")
+ secret, err := dsv.Secret("your secret path")
maybe in this example it should also be the same dsv.Secret("your secret path")
.
Or make path to a Secret configurable (via env or flags).
@@ -117,18 +131,43 @@ func (v Vault) accessResource(method, resource, path string, input interface{}) | |||
return data, err | |||
} | |||
|
|||
type requestBody struct { |
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.
requestBody
is too global (or universal). Please choose a better name e.g. "accessTokenRequest"
Password string `json:"password"` | ||
ClientID string `json:"client_id"` | ||
ClientSecret string `json:"client_secret"` | ||
RefreshToken string `json:"refresh_token"` |
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.
Looks like "RefreshToken" field is not used.
t.Error("unexpected err", err) | ||
} | ||
|
||
if !reflect.DeepEqual(header, tt.expectedHeader) { |
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
and tt.expectedHeader
are strings. No need to use reflect
package to compare them.
t.Error("unexpected header", header) | ||
} | ||
|
||
if !reflect.DeepEqual(body, tt.expectedBody) { |
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.
body
and tt.expectedBody
are strings. No need to use reflect
package to compare them.
@@ -1,3 +1,7 @@ | |||
module github.com/thycotic/dsv-sdk-go | |||
|
|||
go 1.13 | |||
go 1.18 |
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.
awesome. Please change version in ".github/workflows/tests.yml" too
GCP | ||
AZURE |
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 believe these should be added once supported.
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.
Please update README too