Skip to content
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

DC/OS authentication token expiration #292

Open
willgorman opened this issue Jun 26, 2017 · 5 comments
Open

DC/OS authentication token expiration #292

willgorman opened this issue Jun 26, 2017 · 5 comments

Comments

@willgorman
Copy link

The tokens issued by DC/OS that are accepted in the DCOSToken config have an expiration date. This means that users need to manage checking for expiration and obtaining a new token themselves. It would be nice if go-marathon could be configured with credentials (either user/password or service account key) and internally manage obtaining and refreshing tokens.

@timoreimann
Copy link
Collaborator

FWIW, I know that the Google cloud libraries have similar needs for refreshing oauth tokens. The way it's handled there is that there's a library sitting on top of the actual cloud libraries responsible for refreshing the token periodically. It's implemented in terms of a custom HTTP transport layer that does the necessary checking behind the scene and is injected into each cloud library, making the effort completely transparent.

We may want to consider doing something similar. go-marathon already supports injecting a custom HTTP client, so it looks like most of the work could happen in a third-party library / application. This would enable reusal of the token management code across potentially multiple libraries.

I'm not sure if this would be the ideal way to meet the issue's needs, but I'd like to consider it.

@mtweten
Copy link

mtweten commented Jul 13, 2017

I started diving into implementing something like how the golang/oauth2 library implements the custom http transport that you mentioned, until I ended up discovering the dcos-go repo, which contains a custom http transport that serves pretty much the same purpose (it is used in a couple Mesosphere projects that are written in go - dcos-metrics and dcos-log).

Example usage:

privateKey := "-----BEGIN PRIVATE KEY----- ..."

httpClient := &http.Client{}
rt, err := transport.NewRoundTripper(httpClient.Transport,
                  transport.OptionCredentials("service-account", privateKey, "https://dcoshost/acs/api/v1/auth/login"),
                  transport.OptionTokenExpire(time.Duration(time.Hour * 24 * 5)))
if err != nil {
    ...
}

httpClient.Transport = rt
config.HTTPClient = httpClient
marathonClient, err := marathon.NewClient(config)

Couple of potential issues:

  • Only supports uid/private key (not uid/password). Essentially, only DC/OS service accounts are supported.
  • Refreshes the token when a 401 http response is encountered, instead of checking the expiry on the token (not really a big deal).
  • Currently, the transport package only works with v2.6.0 of github.com/dgrijalva/jwt-go - the latest version, v3.0.0, has some breaking API changes.
    • One of the major use cases we wanted this change for is traefik, which currently vendors v3.0.0 of github.com/dgrijalva/jwt-go
    • The changes to support v3.0.0 aren't massive, so we could submit a PR to Mesosphere and see what happens.
  • Not sure what the intent of the dcos-go repository is
    • Is it primarily meant to be used internally? How supported is it? There isn't much documentation, or specific tags indicating some sort of release/versioning scheme.
  • Users of go-marathon would need to be made aware of the package and how to use it.

Seems like it COULD be nice to provide some kind of first class support in go-marathon, such that consumers could just supply configuration and go-marathon would take care of using the custom marathon transport, but maybe that kind of tight coupling is something to be avoided? (warning, my total go language and ecosystem experience totals about 2 days).

Thoughts?

@timoreimann
Copy link
Collaborator

timoreimann commented Jul 18, 2017

One thing to watch out for when considering to add support directly into go-marathon: We are a library, which means we should not vendor dependencies ourselves. (More strictly speaking, we can only vendor hermetically sealed dependencies guaranteed to be not used by our clients, which isn't the case here since we explicitly want clients to be able to use the third-party package.) Consequently, we must always stay compatible with the latest master versions of each and every dependency.

So far, this hasn't been too much of a problem since we only have three or so dependency which break very rarely. Bringing in a dependency from Mesosphere which depends on another library will certainly complicate things. Further aggravating, dcos-go looks more like a binary than a library to me, which means it's free to vendor its dependencies and not necessarily bother with staying up to date with upstream github.com/dgrijalva/jwt-go until they really see an urgent need to update.

Given the circumstances and the somewhat deficient state of dependency management in Go, I feel inclined to leave it to the go-marathon users to vendor everything properly; whether that means trying to vendor a binary-ish application like dcos-go together with jwt-go, choose a different library, or maybe even convince the dcos-go maintainers to split out dcos/http/transport as a proper library on its own that they could then use, would be a choice totally up to the client.

FWIW, if we knew of a working combination, I'd be happy to extend our documentation and leave helpful pointers.

@mtweten
Copy link

mtweten commented Aug 16, 2017

Sorry I hadn't look back on this in awhile. As you saw I went ahead and opened issue #1963 in traefik to hopefully move the discussion forward there. I definitely agree that adding that functionality to go-marathon is problematic given the dependency situation.

The only other option I could think of would be replicating the dcos-go library functionality within go-marathon itself (and removing the dependency on jwt-go), but then that would just add more to be maintained in this project.

@timoreimann
Copy link
Collaborator

@mtweten appreciate you driving forward a solution in Traefik. 👍

Increasing the maintenance burden on go-marathon isn't something I'd totally rule out, but it's certainly not the first route I'd like to pursue.

Ideally, I think there should be a proper third-party library providing just the kind of token refresh functionality we need, where by proper I mean a dedicated package not embedded in a larger binary that follows the usual library vendoring guidlines. It might be worth pinging the Mesosphere maintainers to see if they are interested in the necessary refactoring. Alternatively, we could try to look for another library.

Finally, there's also the option of someone writing the library (either from scratch of by copying it from somewhere with a sufficiently permissive licence) so that go-marathon can leverage it. That'd be nice from a design point of view as well as leaving maintenance ownership with the party that likely has the biggest interest.

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

No branches or pull requests

3 participants