Auth: introduce ec2 credentials auth support #1900
Conversation
kayrus
changed the title
|
Build succeeded.
|
|
Build succeeded.
|
kayrus
changed the title
|
@kayrus This is looking good - I don't see any issues so far. As for unit tests, you can always test the various functions and methods without using mock http requests. For example, make direct calls to As you begin to finalize the code, it might be useful to add further documentation about what each function is doing. You can also link directly to the keystoneclient code as reference. |
|
Build succeeded.
|
|
Build succeeded.
|
|
@jtopjian I added an ability to define a timestamp and a random hash, this allowed to perform unit tests. Also it is possible passthrough auth parameters with a signature without knowing the secret key. I think the code is ready for review now. As for the acceptance tests, I'll add them after I introduce ec2 credentials management. You can test ec2 auth if you create them manually using |
|
I forgot that credentials logic was already introduced in #1460 There are two endpoints available in keystone to manage ec2 credentials:
Creating ec2 credentials via I'll create acceptance tests soon. |
There was a problem hiding this comment.
@kayrus Thank you for working on this -- everything appears correct.
The one area I want to work on is with the overall appearance. Since we're re-using a lot of calculations from keystoneclient, I want to make sure they are noted so other developers have an easy upstream reference. Adding doc strings and more descriptive names will also help with others understanding the code.
With regard to unit tests and the random generation, isn't it possible to seed random with a constant so that the outcome is the same each time?
Please let me know if you have any questions.
it is already done with an empty string: kayrus@9f7ec65#diff-e00b786e8cb6f9ca02483223960d5978R83 I can add a static |
kayrus
force-pushed
the
ec2auth
branch
3 times, most recently
from
37c677f
to
a4ae516
Compare
|
@jtopjian code review acked. |
|
Build succeeded.
|
|
Build succeeded.
|
|
Hold on, there is an issue with GET requests. I had not use the "Params", they are used only for GET requests. |
|
Build succeeded.
|
|
@jtopjian now it should be fine |
There was a problem hiding this comment.
@kayrus This looks really good to me - nice work. Everything appears to be working (acceptance test passes in my environment) and the code is readable and able to be referenced in case someone spots an error or wants to do further work on it. Thank you for your work on this!
| c["signature"] = EC2CredentialsBuildSignatureV4(*opts, signedHeaders, date, c["body_hash"].(string)) | ||
|
|
||
| h["X-Amz-Date"] = date.Format(EC2CredentialsTimestampFormatV4) | ||
| h["Authorization"] = fmt.Sprintf("%s Credential=%s/%s/%s/%s/%s, SignedHeaders=%s, Signature=%s", |
There was a problem hiding this comment.
@jtopjian what if we move Authorization header generation into a func?
There was a problem hiding this comment.
If I'm understanding correctly, all this is doing is building a string, right? And a function for this would have to have several parameters passed into it. Unless I'm misunderstanding the idea you have, I think this is fine the way it is.
There was a problem hiding this comment.
diff --git a/openstack/identity/v3/extensions/ec2tokens/requests.go b/openstack/identity/v3/extensions/ec2tokens/requests.go
index 9be866d1..8712740c 100644
--- a/openstack/identity/v3/extensions/ec2tokens/requests.go
+++ b/openstack/identity/v3/extensions/ec2tokens/requests.go
@@ -180,6 +180,20 @@ func EC2CredentialsBuildSignatureV4(opts AuthOptions, signedHeaders string, date
return hex.EncodeToString(sumHMAC256(key, []byte(strToSign)))
}
+// EC2CredentialsBuildAuthorizationHeaderV4 builds an AWS v4 Authorization
+// header based on auth parameters, date and signature
+func EC2CredentialsBuildAuthorizationHeaderV4(opts AuthOptions, signedHeaders string, signature string, date time.Time) string {
+ return fmt.Sprintf("%s Credential=%s/%s/%s/%s/%s, SignedHeaders=%s, Signature=%s",
+ EC2CredentialsAwsHmacV4,
+ opts.Access,
+ date.Format(EC2CredentialsDateFormatV4),
+ opts.Region,
+ opts.Service,
+ EC2CredentialsAwsRequestV4,
+ signedHeaders,
+ signature)
+}
+
// ToTokenV3ScopeMap is a dummy method to satisfy tokens.AuthOptionsBuilder interface
func (opts *AuthOptions) ToTokenV3ScopeMap() (map[string]interface{}, error) {
return nil, nil
@@ -239,24 +253,14 @@ func (opts *AuthOptions) ToTokenV3CreateMap(map[string]interface{}) (map[string]
if opts.Timestamp != nil {
date = *opts.Timestamp
}
+ h["X-Amz-Date"] = date.Format(EC2CredentialsTimestampFormatV4)
if v, _ := c["body_hash"]; v == nil {
// when body_hash is not set, generate a random one
c["body_hash"] = randomBodyHash()
}
-
signedHeaders, _ := h["X-Amz-SignedHeaders"]
c["signature"] = EC2CredentialsBuildSignatureV4(*opts, signedHeaders, date, c["body_hash"].(string))
-
- h["X-Amz-Date"] = date.Format(EC2CredentialsTimestampFormatV4)
- h["Authorization"] = fmt.Sprintf("%s Credential=%s/%s/%s/%s/%s, SignedHeaders=%s, Signature=%s",
- EC2CredentialsAwsHmacV4,
- opts.Access,
- date.Format(EC2CredentialsDateFormatV4),
- opts.Region,
- opts.Service,
- EC2CredentialsAwsRequestV4,
- signedHeaders,
- c["signature"])
+ h["Authorization"] = EC2CredentialsBuildAuthorizationHeaderV4(*opts, signedHeaders, c["signature"].(string), date)
return b, nil
}There was a problem hiding this comment.
Yeah, a single-line function feels unnecessary to me.
Resolves #1898
@jtopjian before I introduce unit tests please take a look on this and let me know your opinion. It is hard to introduce unit tests, since token signature changes every call. I'd prefer to introduce an interface to create, list, delete ec2 credentials, then integrate them with ec2 auth.