Skip to content
This repository has been archived by the owner on Aug 1, 2023. It is now read-only.

Fix keystone v3 token auth #528

Merged
merged 3 commits into from
Jun 17, 2016

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Feb 2, 2016

Currently keystone v3 client ignores TokenID in AuthOptions and the code like this

provider, err := openstack.AuthenticatedClient(gophercloud.AuthOptions{
    IdentityEndpoint: "https://.../v3/",
    TokenID:          "abcd...1234",
})

fails with an error You must provide a password to authenticate because options.TokenID is never checked. (keystone v2 client checks it in ToTokenCreateMap.) This patch lets authentication succeed.

I am new to the codebase and do not understand if this issue should be fixed like this, and I would also appreciate help in writing a test.

@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch from 05bb111 to dc00bd6 Compare February 2, 2016 23:22
@orivej
Copy link
Contributor Author

orivej commented Feb 2, 2016

In the second patch I enabled scoped token auth as in:

provider, err := openstack.AuthenticatedClient(gophercloud.AuthOptions{
    IdentityEndpoint: "https://.../v3/",
    TokenID:          "abcd...1234",
    DomainID:         "default",
    TenantName:       tenant,
})

which is documented at http://developer.openstack.org/api-ref-identity-v3.html#authenticateTokenScoped

@jrperritt
Copy link
Contributor

The second patch looks good.

For the first, v3 auth sets the Token here. Instead of c.TokenID I think changing it to opts.TokenID would do it, no? Also, line 87 would need the same change.

@orivej
Copy link
Contributor Author

orivej commented Apr 15, 2016

Instead of c.TokenID I think changing it to opts.TokenID would do it, no?

Create seems to be supposed to be able to reauthenticate existing clients, using token cached in c.TokenID. This is tested by authTokenPost and other tests.

You can think of client.TokenID = options.TokenID as prepopulating token that would be returned to the client if it authenticated with login and password.

Although with my patches all tests pass, I think we should replace proposed

client.TokenID = options.TokenID

with

if len(options.TokenID) != 0 {
    client.TokenID = options.TokenID
}

and move it out of the switch.

@orivej
Copy link
Contributor Author

orivej commented Apr 15, 2016

Disregard the suggestion of moving it out of the switch: v2 client already processes options.TokenID and seems to ignore client.TokenID.

@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch from dc00bd6 to 43ade91 Compare April 15, 2016 15:30
@orivej
Copy link
Contributor Author

orivej commented Apr 15, 2016

I moved the original proposal to https://github.com/orivej/gophercloud/tree/openstack-identity-v3-pass-tokenid%401 and improved the first patch by relocating token assignment into v3/tokens/request.go.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 80.162% when pulling 43ade91 on orivej:openstack-identity-v3-pass-tokenid into 231898e on rackspace:master.

@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch 2 times, most recently from 5b259d7 to e18873e Compare April 15, 2016 15:54
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 80.162% when pulling e18873e on orivej:openstack-identity-v3-pass-tokenid into 231898e on rackspace:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 80.162% when pulling e18873e on orivej:openstack-identity-v3-pass-tokenid into 231898e on rackspace:master.

@orivej
Copy link
Contributor Author

orivej commented Apr 15, 2016

Rebased onto master, combined into one commit, and added a test that covers the addition to requests.go. I do not know why coveralls reports that it is not covered.

@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch from e18873e to 1a23ff6 Compare April 15, 2016 16:15
@orivej
Copy link
Contributor Author

orivej commented Apr 15, 2016

Updated ErrMissingPassword message.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 80.162% when pulling 1a23ff6 on orivej:openstack-identity-v3-pass-tokenid into 231898e on rackspace:master.

@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch from 1969e66 to 1a23ff6 Compare April 15, 2016 16:34
@orivej
Copy link
Contributor Author

orivej commented Apr 15, 2016

The coverage decreases because github.com/rackspace/gophercloud/openstack/identity/v3/tokens tests do not cover the addition to requests.go, but github.com/rackspace/gophercloud/openstack do cover it.

@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch from 1a23ff6 to 16193fd Compare April 15, 2016 20:05
@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch from 16193fd to bb30330 Compare April 15, 2016 20:08
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 80.174% when pulling bb30330 on orivej:openstack-identity-v3-pass-tokenid into 231898e on rackspace:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 80.174% when pulling bb30330 on orivej:openstack-identity-v3-pass-tokenid into 231898e on rackspace:master.

@orivej
Copy link
Contributor Author

orivej commented Apr 15, 2016

I have moved the new test into request_test.go, and split the patch back in two.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 80.164% when pulling 5befda5 on orivej:openstack-identity-v3-pass-tokenid into 231898e on rackspace:master.

@orivej orivej force-pushed the openstack-identity-v3-pass-tokenid branch from 5befda5 to 3d6ab64 Compare April 17, 2016 00:47
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 80.164% when pulling 3d6ab64 on orivej:openstack-identity-v3-pass-tokenid into c54bbac on rackspace:master.

@@ -30,16 +31,17 @@ func AuthOptionsFromEnv() (gophercloud.AuthOptions, error) {
tenantName := os.Getenv("OS_TENANT_NAME")
domainID := os.Getenv("OS_DOMAIN_ID")
domainName := os.Getenv("OS_DOMAIN_NAME")
tokenID := os.Getenv("OS_TOKEN")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand the use-case for this? Since tokens are presumably ephemeral, why would we want to load them from an environment variable? And if a user has set OS_TOKEN, what then when the token inevitably expires?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://specs.openstack.org/openstack/keystone-specs/api/v3/identity-api-v3-os-oauth1-ext.html says that OAuth tokens

Provide the ability for identity users to delegate roles to third party consumers via the OAuth 1.0a specification. Requires v3.0+ of the Identity API. An OAuth-derived token will provide a means of acting on behalf of the authorizing user.

They can be revoked by the user or expire (which is optional), after which they are meant to no longer provide authorization to perform on behalf of the user.

@jrperritt
Copy link
Contributor

LGTM. +2

@jrperritt jrperritt merged commit f28452d into rackspace:master Jun 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants