1628611 - Taskcluster Hook tokens seem hard to audit.

Status: NEW

(Taskcluster :: Services, enhancement)

Description

[Tom Prince [:tomprince]]

I was looking triggering taskcluster hooks with a token, to easily trigger a hook from cloudops' Jenkins instance, and so I started looking at how they were managed.

When a hook is created, a token is associated with that hook. There are API calls to retrieve the token, and reset it.

Thus, there does not seem to be a way to see which hooks have tokens in use (since there is a token for every hook).

Given that the scope to retrieve a scope is specific to that, and is not granted beyond root scopes in firefox-ci, I don't think this is a time-sensitive issue.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Can you be more specific? We could certainly look for calls to triggerHook in the logs to audit these, and the last-fires data (available via API) also captures this information.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

*triggerHookWithToken

Comment 3

[Tom Prince [:tomprince]]

I think the summary is that I'm not sure how hook tokens are intended to be managed, and in particular how to track usage with something like ci-admin/tc-admin. I've have some more detailed thoughts below about my thought processes, by I lack the context of the original design.

Having dug into cloudops infra some more, I think I'm going to end up using tc clients for what I was investigating anyway.

---

So, it may be that I had incorrect assumptions about how things would work, that are coloring my reactions. I had originally thought that getTriggerToken would get a fresh token and resetTriggerToken would delete it, then went looking to how to find out how to find the outstanding tokens, which lead to my description above.

Certainly we could use the audit logs for getTriggerToken to see which hooks have had tokens that could be used, but it feels like that is something that should need access to the audit logs.

One minimal change that would address my concerns at least to a degree, is to have hooks default to not having a token, and have getTriggerToken/resetTriggerToken create and/or delete the token respectively. Then querying the hook could return whether their is a token. Alternatively, it could be a flag to create/modify, whether there is a token, that get/reset then work only for those hooks that have the flag set.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

It is a bit of an old interface, and different from our others. Probably if we were to design it today there would only be setTriggerToken and clearTriggerToken, with the first creating a new, random token and returning it (once) and the second clearing that token and disabling use of a token to trigger the hook. Then we could also indicate the presence of a token (but not the token itself) in hooks.hook. ci-admin could enforce that boolean value is false where it should be, without ever having to handle tokens.

That's not a huge change, and could be done without breaking the existing methods (just deprecating them). It's also probably not a high priority. Could we open this bug up and mark it as mentored?

Comment 5

[Tom Prince [:tomprince]]

(In reply to Dustin J. Mitchell [:dustin] (he/him) from comment #4)

Could we open this bug up and mark it as mentored?

I guess as neither cluster grants the scopes to get tokens, opening it up is fine.

It is a bit of an old interface, and different from our others. Probably if we were to design it today there would only be setTriggerToken and clearTriggerToken, with the first creating a new, random token and returning it (once) and the second clearing that token and disabling use of a token to trigger the hook. Then we could also indicate the presence of a token (but not the token itself) in hooks.hook. ci-admin could enforce that boolean value is false where it should be, without ever having to handle tokens.

That's not a huge change, and could be done without breaking the existing methods (just deprecating them).

I suspect there is not a need to deprecate the existing methods, just remove (or change) them, since they aren't used as far as I can tell.

It's also probably not a high priority.

Definitely isn't.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Thanks! So the task for this bug is outlined in comment 4.

It's true that we could just remove the old methods, but I think it will be relatively easy to leave them in place, and that means anyone we're not aware of that might be using them has time to adapt.

Comment 7

[Tom Prince [:tomprince]]

Given that the only people in either cluster with the scopes in either cluster are releng/relops, I think the danger of breaking anybody is small. And also would let us learn about them, so that we can track that usage.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

That's true, ok, we can just delete the old methods :)