Created attachment 8634192 [details] [review] PR for old taskcluster-client-go repo
Assignee: nobody → pmoore
Status: NEW → ASSIGNED
Attachment #8634192 - Flags: review?(jopsen)
I'm curious why this is required? Would it ever be relied on for an actual security decision, or is it just a way to decide whether or not to show a UI element based on whether the user has the necessary scopes?
Comment on attachment 8634192 [details] [review] PR for old taskcluster-client-go repo See comments, I think this is doing too many fancy things... This should really just be a set of nested loops, going all the way into byte comparison.
Attachment #8634192 - Flags: review?(jopsen) → review-
@dustin, I suspect we have some use-cases in generic-worker. It's not that generic-worker check if a task has scopes to make an API call (it'll used authorizedScopes for this). But if generic-worker has features that we don't want to expose to everybody, protecting them behind a scope often makes sense. Example could be interactive features, where we export an artifact with a password for the RDP session within which the task lives. Or access to caches, which are protected behind scopes in docker-worker. Ie. docker-worker checks that task.scopes has the scope: docker-worker:cache:<cacheName> Before granted a task access to an existing (or create a new) cache folder with the name <cacheName>.
Hm, I'd argue it should be in taskcluster-base-go, in that case, rather than taskcluster-client-go, if only to mirror the JS libraries. In general, I think of a client as something external to taskcluster (so on the outside requesting access via scopes). Generic-worker does need to control access to features based on scopes, as you've described. But I think it does so as a taskcluster component, and as such should depend on a library of common component functionality -- taskcluster-base(-go). It's fine (and a great design) that generic-worker also act as a client, I just see that as distinct from the functionality developed here.
There is a more obvious implemetation here: https://play.golang.org/p/-OWB--4fIb @dustin, I see your point. But I don't think we're going to reimplement taskcluster-base in other languages anytime soon.
So this is parallel to the use of scopeMatch in docker-worker to determine whether the given caches are allowed. I don't really see the harm in creating taskcluster-base-go (even if it just has this one method, rather than a re-implementation of taskcluster-base), but having spoken my piece I'll shut up now :)
Hi guys, Thanks for all your feedback! I'm totally aligned that Jonas's implementation is superior. I also agree the function doesn't belong in the client. In line with http://blog.golang.org/package-names (a great post, well worth reading) I'll create a taskcluster scopes package, with this single function and the associated go types. I think we should differentiate between given scopes, and required scopes, as types, since they have different handling. A given scope does not attach significance to a trailing '*', whereas a required scope does. It therefore /feels/ unsafe to pass a ScopeSet around that could be a given scopeset in one place, and a required scopeset elsewhere. Or is this overkill? In the taskcluster-base js implementation, the function isn't defined against a type, so I think scopeMatch could be an appropriate name, but since in the go implementation the function is defined against the type, I like having the function name "Satisfies" since a.satisfies(b) seems more intuitive than a.scopeMatch(b). Is that ok?
(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #6) > There is a more obvious implemetation here: > https://play.golang.org/p/-OWB--4fIb This doesn't enforce that the '*' matching occurs at the end of the scope, i.e. see https://play.golang.org/p/lpeVKGIpF6 This therefore behaves differently to: https://github.com/taskcluster/taskcluster-base/blob/218225942212e24596cee211389c276b2b985ffe/utils.js#L61 I'm not sure if we have a formal definition of scopeset satisfaction to validate which of the two implementations is correct, but I'll make the go implementation consistent with the node.js one for now.
We do have a formal definition -- the one given in the scopes presentation. And I think the set of tests for the JS version is reasonably exhaustive (although it doesn't verify that * is not expanded within a string.. I should add that) As for differentiating scopes and scopesets, I think you get that for free in Go, since the former is a list of strings while the latter is a list of lists of strings. As for the name, if you think of it as a function on two arguments, then scopeMatch makes sense. I think that Scopes.satisfies could *call* scopeMatch. In fact, that's more or less what the JS implementation does (where req.satisifies is a simple wrapper around scopeMatch).
Created attachment 8642504 [details] [review] PR for new taskcluster-base-go repo Moved to a new component, and incorporated feedback. Thanks guys!
Attachment #8634192 - Attachment description: Pull Request → PR for old taskcluster-client-go repo
Attachment #8642504 - Flags: review?(dustin) → review+
(In reply to Dustin J. Mitchell [:dustin] from comment #10) > We do have a formal definition -- the one given in the scopes presentation. Ah great point, I'd forgotten about that! I'll link to that in docs and README.md... > And I think the set of tests for the JS version is reasonably exhaustive > (although it doesn't verify that * is not expanded within a string.. I > should add that) I will also look at the tests you have and make sure I have the same ones. I wonder if we should share a common set of language-independent tests (e.g. publish tests as json somewhere, and consume it to generate tests). Or ... overkill? I think this was done in e.g. docopt - there I believe they have language independent unit tests (kinda cool, hey). > As for differentiating scopes and scopesets, I think you get that for free > in Go, since the former is a list of strings while the latter is a list of > lists of strings. This was just in relation to the playground example that Jonas created - in there, given scopes was called ScopeSet and was a string array, and required scopes was called ScopeSets and was a ScopeSet array (rather than a string array array). The nice part was you could say a ScopeSet is satisfied if all the scopes inside it are satisfied, and a ScopeSets is satisfied if it has a ScopeSet that is satisfied - I just didn't like the dual purpose of a ScopeSet representing both given scopes and an element of required scopes, due to the '*' handling being different. I think they happen to share the same structure (string array) but semantically serve (slightly) different purposes. To that end, not reusing the type seemed appropriate to me. That said, we could have: type ( Given string scopeSet string Required scopeSet ) This way the handling of scope sets is internal to the package (not exported due to lower-case leading 's' in scopeSet) and a scopeSet can (internally within the package) be said to be satisfied or not. On reflection, this might be nice. I'll implement and then it will be easier to decide if it is cleaner/nicer/better or too noisy. > As for the name, if you think of it as a function on two arguments, then > scopeMatch makes sense. I think that Scopes.satisfies could *call* > scopeMatch. In fact, that's more or less what the JS implementation does > (where req.satisifies is a simple wrapper around scopeMatch). I think it might be nicer to only expose one function in the package for the sake of simplicity, but we could move it into a function later if there was a need for it (in which case I would call it "Match" rather since you would call it with scopes.Match(a, b) rather than scopes.ScopeMatch(a, b)). At the moment I can't think of a case where calling scopes.Match(a, b) offers advantages over calling a.Satisfies(b).
(In reply to Pete Moore [:pmoore][:pete] from comment #12) > (I'll implement and then it will be easier to decide if it is > cleaner/nicer/better or too noisy. @jonasfj: @dustin: I pushed to a different branch so you can compare: https://github.com/taskcluster/taskcluster-base-go/blob/bug1184044_v2/scopes/scopes.go I'm not sure which I prefer. Probably the original as it is more concise, but I'm open to your opinions! =)
I've now completed the other parts. Commits pushed to existing PR. =) 1) Added unit test coverage reporting to https://coveralls.io/github/taskcluster/taskcluster-base-go 2) Ported over node.js unit tests from https://raw.githubusercontent.com/taskcluster/taskcluster-base/master/test/scopematch_test.js 3) Updated README.md with coverage report link, improved scope example to contain `*` semantics, linked to formal scope definitions, and corrected IRC link 4) Corrected inaccurate test comment (the "a" vs "b" comment highlighted by dustin) 5) godoc updates: i) linked to formal scopes definitions from package comments, ii) linked to original scopeMatch implementation in Satisfied function, iii) explained that `*` can be used in required scopes iv) added a godoc "Example" with required scopes ending with `*`
Comment on attachment 8642504 [details] [review] PR for new taskcluster-base-go repo The code looks good to me. Instead of calling the types for Given and Required I might choose ScopesSet and ScopeSets, but it's a detail (only relevant if consumers sees this). I like the code...
Attachment #8642504 - Flags: review?(jopsen) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
"ScopesSet" vs. "ScopeSets".. that's a rather difficult-to-discern distinction! While "Given" and "Required" are not terms used in JS implementations, they do add a nice level of explicitness. I think the use-case for moving scopes between the two sides of this "satisfies" relationship is `task.scopes`: you need to verify `scopeMatch(request_scopes, [task.scopes])` when the task is created, then `scopeMatch(task.scopes, [["some:scope:foo/bar"]])` when performing an action on behalf of that scope. Ideally the latter would be validated in the form of an API call and using authorizedScopes, but we've seen other cases in docker-worker (devices and caches). As long as it's fairly easy to cast an array of strings into Given, I think these names are good. It's a shame that interfaces don't actually help here (at least, not without adding more complexity).
You need to log in before you can comment on or make changes to this bug.