Bug 1235399 - enable tc-proxy to work with certificates #8
Conversation
|
In order to test this in travis, we'll need to implement http://docs.taskcluster.net/auth/temporary-credentials/ in go - that hasn't been done yet, but should live in taskcluster-client-go. |
|
Note, I logged in just now with login.taskcluster.net, and got myself some temporary credentials. I then used these for running the tests, and all the tests passed. \o/ We should still do this as part of the tests - but just so you know, it does seem to be working, even if we don't have unit tests yet. |
…t 3 numbers. Since supporting a new feature now (temp creds), bumped second number rather than third.
…t 3 numbers. Since supporting a new feature now (temp creds), bumped second number rather than third.
|
Note - the travis failure is only because coverage has reduced overall with this change. When I add tests for temp creds, this will obviously be resolved. |
| and `TASKCLUSTER_ACCESS_TOKEN` environment variables. | ||
| and `TASKCLUSTER_ACCESS_TOKEN` environment variables, optionally with | ||
| a `TASKCLUSTER_CERITIFICATE` environment variable in the case of using | ||
| temporary credentials. |
There was a problem hiding this comment.
Note that at least with docker, you do NOT want to communicate these with env vars, as those vars get injected into every linked container (including the task container). Docker-worker uses the command-line options instead.
|
Looks good :) |
|
Although the latest commit appears like a big refactor, I essentially just inlined the tests as a local function, and then call it with both permanent credentials and temporary credentials. Due to the increased indentation, the diff looks like quite a mess, but essentially that's all I did... |
|
@djmitche can you take a look again? Thanks! |
|
|
||
| func testWithTempCreds(t *testing.T, test IntegrationTest) { | ||
| skipIfNoPermCreds(t) | ||
| tempCredentials, err := permCredentials.CreateTemporaryCredentials(1*time.Hour, "*") |
There was a problem hiding this comment.
Whoops - I forgot to reduce this from '*' - I still need to do that (will do that tomorrow)...
|
Looks good |
…ther with temp creds in current implementation
|
I updated the test - and discovered a bug in the process! In short, currently AuthorizationDelegate function is not working with temporary credentials - the easiest way to fix this will be to use tcclient library instead. Basically, the ext field in the Authorization header doesn't include both the temporary credentials certificate and the authorizedScopes - they need to be merged into a single json. The tcclient library does this. |
…ork with authorized scopes combined with temporary credentials
|
Fixed the genuine bug that was discovered via a test. With the updates, taskcluster-proxy is now using the same tcclient library as taskcluster-client-go uses under the hood, so we now have less code to manage @djmitche hopefully this should be the last review! Thanks. |
|
👍 |
Bug 1235399 - enable tc-proxy to work with certificates
Not yet tested...