I don't think we have a "client password" in this case -- I believe that refers to a mode that no one uses for anything (well, maybe a mode that Concourse uses, I guess), where the flow is actually handling normal user passwords?

e.g., section 11.2.2 discusses password, separate from client_id or client_secret and section 1.3.3 describes a "Resource Owner Password Credentials" mode (which is not a mode we operate in).

If "password" does mean "client secret", we're out of compliance with this:

The [client credential] parameters can only be transmitted in the request-body and MUST NOT be included in the request URI.

...but so are most other other OAuth2 implementations, as far as I can recall from the half dozen or so I've written bindings for.

(Practically, there are other cases where we explicitly deviate from the spec, so strict spec compliance isn't a goal, per se.)

I think this change isn't necessarily unreasonable, but I'd like to understand your overall flow better. I don't normally expect OAuth to be useful for connecting CI to Phabricator -- my general expectation is that OAuth is used when systems need to act on behalf of individual users, but in most situations I usually expect CI to act as a single bot/build user.

Are you actually setting things up so that individual users use OAuth to authenticate with the build server? e.g., everyone has to log in and connect their build server account to their Phabricator account to enable builds?

If you aren't, and are just trying to grant credentials to a single CI bot/build user, what problem are you hoping to solve by using OAuth in this flow?

It seems like section 2.3.1 is poorly worded. They talk about a "client password" but the example request only includes client_secret as a POST variable. In the case of the Golang OAuth2 module (which Concourse uses), it sets the basic auth username to the client ID and the password to the client secret (https://github.com/golang/oauth2/blob/master/internal/token.go#L164).

In Phabricator's case, it expects client_secret to be the "Application Secret" of the OAuth server (the one hidden behind "Show Application Secret" and the confirmation prompt).

Are you actually setting things up so that individual users use OAuth to authenticate with the build server? e.g., everyone has to log in and connect their build server account to their Phabricator account to enable builds?

Yes. Concourse doesn't maintain a user database of its own (and, hence, has no concept of build server accounts), but instead provides "teams" which serve as namespaces for any CI related objects (resources, pipelines, etc). Each team has a separate authentication method, and the supported methods are:

• A static username/password
• Github-specific OAuth
• CloudFoundry UAA (User Account and Authentication)
• Generic OAuth

I've got each team set up with a different OAuth server application in Phabricator, and then the Concourse team gets set up with its client PHID, application secret, and URIs. Users navigate to Concourse, click "login", select a team, and are redirected to Phabricator to authorise the application (which shows up as "Concourse: Team Main" or whatever), and, after giving permission, are redirected back to Concourse. With D16763 applied, this flow works perfectly.

Is "GitHub-specific OAuth" just normal OAuth with client_id / client_secret as parameters instead of in an "Authorization" header? The GitHub OAuth documentation seems to suggest passing these as parameters, too.

Is there a reason that Concourse / Golang select "Authorization" as the only credential transfer mechanism instead of parameters?

Almost every provider I'm aware of suggests using parameters in practice:

GitHub:

Facebook:

GitLab:

Google:

Actually, Bitbucket does suggest doing it the other way:

But it seems like this is a weird default given that the empirical consensus among providers is to recommend parameters, not "Authorization". As far as I can tell, none but Bitbucket even document that "Authorization" works, although presumably it does if this is the implementation choice.

I've done some more digging as well and it appears that this issue arose in the Go library already: https://github.com/golang/oauth2/issues/111

Takeaways:

• They've added a function to register a host as being "broken" and requiring client_secret passed in the request body instead of in the auth header.
• However, they pressured Github to add the auth header support in Github enterprise (https://enterprise.github.com/releases/2.3.4/notes) as a result of the issue.

So, while I agree that many APIs document that client_secret should be passed as a parameter, this reading of section 2.3.1 matches mine, and it seems like OAuth servers which don't support the basic auth header are off-spec.

I could also expose marking an OAuth endpoint as "broken" in Concourse and pass that off to the OAuth library, which would be a good idea for Concourse. Do you have any particular reservation against landing D16763 or is this just due diligence to see how other providers implement it?

Broadly, I want to avoid implementing things that don't have good technical justifications.

As far as I can tell, the argument on the Go side seems to exclusively be "the spec says so", not "...and it's actually a good idea". I imagine that the spec authors may have been balancing a risk of inadvertent disclosure (e.g., in logs or debugging messages) against other concerns, but in practice the vast majority of implementors (including us) have chosen simplicity over risk reduction. I think this choice is generally a reasonable one, and that the security afforded by use of the "Authorization" header is small to the point of insignificance.

I think this passage makes it clear that the spec does consider client_secret to be a password:

Since this client authentication method [referring to an example above with client_secret] involves a password, the authorization server MUST protect any endpoint utilizing it against brute force attacks.

...and thus the Go implementors are technically correct in their approach. But this outcome is totally ridiculous:

var brokenAuthHeaderProviders = []string{
	"https://accounts.google.com/",
	"https://api.dropbox.com/",
	"https://api.instagram.com/",
	"https://api.netatmo.net/",
	"https://api.odnoklassniki.ru/",
	"https://api.pushbullet.com/",
	"https://api.soundcloud.com/",
	"https://api.twitch.tv/",
	"https://app.box.com/",
	"https://connect.stripe.com/",
	"https://login.microsoftonline.com/",
	"https://login.salesforce.com/",
	"https://oauth.sandbox.trainingpeaks.com/",
	"https://oauth.trainingpeaks.com/",
	"https://oauth.vk.com/",
	"https://slack.com/",
	"https://test-sandbox.auth.corp.google.com",
	"https://test.salesforce.com/",
	"https://user.gini.net/",
	"https://www.douban.com/",
	"https://www.googleapis.com/",
	"https://www.linkedin.com/",
	"https://www.strava.com/oauth/",
}

https://github.com/golang/oauth2/blob/442624c9ec9243441e83b374a9e22ac549b5c51d/internal/token.go#L92

Go is in a tough spot because at least some servers probably do not accept client_id and client_secret, so they either have to pick the technically correct approach, the empirically-more-common approach, or add an option. But I'd be more sympathetic to Go if it looked like they were making a more reasonable effort to accept the reality of the situation -- hard-coding Google, Slack, Dropbox, Stripe, Salesforce, etc., into your API as "broken" seems like it's purely defending a moral highground. I agree that they have the highground, but come on.

This one is particularly ridiculous to me (added 18 months ago):

"https://test-sandbox.auth.corp.google.com",

It looks like the committer who added that is an employee of Google, and paid by Google to work on Golang. I don't work at Google myself, but it seems a little inconsistent to defend this choice as technically correct and mark these implementations as "broken" but then just hack around it instead of fixing it when it's your employer's implementation because that would be too much work and we live in the real world after all.

Anyway, the complexity cost of D16763 is small and I don't think there's any concrete reason not to support "Authorization" (I just don't have any technical argument in favor of it actually being a better choice beyond "the spec says so"). I'm ultimately OK with upstreaming it, I just think Go is a little silly to pick this battle.

...and here are foreseeable issues with this mess occurring in the wild:

https://github.com/golang/oauth2/issues/166#issuecomment-160361483

Ah well. We fight plenty of dumb battles, too, and the fallout of this one is pretty miniscule for us.