Yea, that's a good idea. It's sort of like the 'critical' attribute in X509. "If a reader sees an attribute listed as critical, and it doesn't know what to make of it, don't accept the cert".
I'll add a repeated enum for this even though it will be empty for now. 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 > >> > > > > > -- Todd Lipcon Software Engineer, Cloudera
