On Wed, Jan 25, 2017 at 3:33 PM, Alexey Serbin <[email protected]> wrote: > > Or it's proposed that if new > fields are found in a token then don't accept the token at all? In that > case it might be an issue during rolling upgrade, I think. >
That's what I was thinking, but that's a good reason not to do it that way. Our normal required feature flags mechanism should work in that case, where the field is not critical/required. > > > Best regards, > > Alexey > > On Wed, Jan 25, 2017 at 3:14 PM, Dan Burkert <[email protected]> > wrote: > > > That's an interesting idea - say if the format evolved to have an > > additional field which restricts use of the token? Could we use the > > protobuf unknown fields API to recognize if this happened? > > > > - Dan > > > > On Wed, Jan 25, 2017 at 3:03 PM, Alexey Serbin <[email protected]> > > wrote: > > > >> I like this idea. > >> > >> Probably, that's too early at this point, but consider adding notion of > >> compt/non-compat feature flags into tokens. Imagine the new version of > >> token has some additional restriction field, which older code does not > >> understand. In that case, even if the token signature is correct, the > >> older code should not accept the new token since unsupported non-compat > >> feature flags is present. > >> > >> But in the essence this looks great, IMO. > >> > >> > >> Best regards, > >> > >> Alexey > >> > >> On Wed, Jan 25, 2017 at 12:52 PM, Todd Lipcon <[email protected]> > wrote: > >> > >>> Actually had one more idea... how about: > >>> message AuthenticationToken { > >>> }; > >>> > >>> message AuthorizationToken { > >>> }; > >>> > >>> message TokenPB { > >>> // The time at which this token expires, in seconds since the > >>> // unix epoch. > >>> optional int64 expire_unix_epoch_seconds = 1; > >>> > >>> oneof token { > >>> AuthenticationToken authn = 2; > >>> AuthenticationToken authz = 3; > >>> } > >>> }; > >>> > >>> message SignedTokenPB { > >>> // The actual token contents. This is a serialized TokenPB protobuf. > >>> However, we use a > >>> // 'bytes' field, since protobuf doesn't guarantee that if two > >>> implementations serialize > >>> // a protobuf, they'll necessary get bytewise identical results, > >>> particularly in the > >>> // presence of unknown fields. > >>> optional bytes token_contents = 2; > >>> > >>> // The cryptographic signature of 'token_contents'. > >>> optional bytes signature = 3; > >>> > >>> // The sequence number of the key which produced 'signature'. > >>> optional int64 signing_key_seq_num = 4; > >>> }; > >>> > >>> > >>> > >>> On Wed, Jan 25, 2017 at 12:44 PM, Todd Lipcon <[email protected]> > wrote: > >>> > >>>> On Wed, Jan 25, 2017 at 12:40 PM, Dan Burkert <[email protected]> > wrote: > >>>> > >>>>> I think it must go in the 'token_contents' itself, otherwise it can > be > >>>>> modified by a malicious client. Other than that, looks good. > >>>>> > >>>>> > >>>> well, if it went into a separate field, then we'd have something like: > >>>> > >>>> optional bytes token_contents = 1; > >>>> optional int64 expiration_timestamp = 2; > >>>> > >>>> // Signature of the string: '<32-bit big-endian length of > >>>> token_contents> token_contents <64-bit big-endian expiration>' > >>>> optional bytes signature = 3; > >>>> > >>>> so they could try to modify it, but the signature would fail. > >>>> > >>>> > >>>> > >>>>> On Wed, Jan 25, 2017 at 12:37 PM, Todd Lipcon <[email protected]> > >>>>> wrote: > >>>>> > >>>>>> Hey folks > >>>>>> > >>>>>> I'm working on the token signing/verification stuff at the moment. > >>>>>> Curious to solicit some opinions on this: > >>>>>> > >>>>>> > >>>>>> message TokenPB { > >>>>>> // The actual token contents. This is typically a serialized > >>>>>> // protobuf of its own. However, we use a 'bytes' field, since > >>>>>> // protobuf doesn't guarantee that if two implementations > serialize > >>>>>> // a protobuf, they'll necessary get bytewise identical results, > >>>>>> // particularly in the presence of unknown fields. > >>>>>> optional bytes token_contents = 1; > >>>>>> > >>>>>> // The cryptographic signature of 'token_contents'. > >>>>>> optional bytes signature = 2; > >>>>>> > >>>>>> // The sequence number of the key which produced 'signature'. > >>>>>> optional int64_t signing_key_seq_num = 3; > >>>>>> }; > >>>>>> > >>>>>> The thing that's currently missing is an expiration timestamp of the > >>>>>> signature. I have two options here: > >>>>>> > >>>>>> *Option A*) say that the TokenPB itself doesn't capture expiration, > >>>>>> and if a particular type of token needs expiration, it would have > to put an > >>>>>> 'expiration time' in its token contents itself. > >>>>>> > >>>>>> *pros:* > >>>>>> - token signing/verification is just a simple operation on the > >>>>>> 'token_contents' string > >>>>>> > >>>>>> *Cons:* > >>>>>> - would likely end up with redundant code between AuthN and AuthZ > >>>>>> tokens, both of which need expiration. However, that code isn't very > >>>>>> complicated (just a timestamp comparison) so maybe not a big deal? > >>>>>> > >>>>>> *Option B)* add an expiration timestamp field to TokenPB > >>>>>> *pros:* > >>>>>> - consolidate the expiration checking code into TokenVerifier > >>>>>> *cons:* > >>>>>> - now in order to sign/verify a token, we actually need to be > signing > >>>>>> something like a concatenation of 'token_contents + signature'. Not > too > >>>>>> hard to construct this concatenation, but it does add some > complexity. > >>>>>> > >>>>>> Any strong opinions either way? > >>>>>> > >>>>>> -Todd > >>>>>> -- > >>>>>> Todd Lipcon > >>>>>> Software Engineer, Cloudera > >>>>>> > >>>>> > >>>>> > >>>> > >>>> > >>>> -- > >>>> Todd Lipcon > >>>> Software Engineer, Cloudera > >>>> > >>> > >>> > >>> > >>> -- > >>> Todd Lipcon > >>> Software Engineer, Cloudera > >>> > >> > >> > > >
