concourse
RFC: Pipeline Identity Tokens
Would likely recommend against making RSA the default, seems to be a trend of moving towards EC25519 since RS256 uses PKCS#1v1.5 which is the vector for the Bleichenbacher style of attack, not sure increasing the bit size avoids the issues exploited in this style of attack. But should likely be default only, if the implementer is confident they have a closed ecosystem they should be able to specify other signing protocols.
I mainly thought of RS256 because I think it has the widest support out there. RFC7518 lists both RS256 and ES256 as recommended, so compatibility should be similar I guess.
Not sure if RS256 is actually affected by Bleichenbacher attacks. From reading into it it seems to mostly affect encryption via RSA and I couldn't find a source that says RS256 is affected.
I came up with some code to showcase how this feature could work: https://github.com/concourse/concourse/pull/9035
@aliculPix4D @marco-m-pix4d : So, what can be done to go forward with this RFC? What would be the next steps?
Hello @dbaumgarten, both myself and @aliculPix4D are only contributors. Let's ask @taylorsilva if he could please chime in.
Hey folks, I don't have time to go over RFC's right now. No maintainers have reviewed this RFC yet so that'll need to happen next, which basically is me reviewing it. I'll have time to do that once I get the next release out.
@taylorsilva I really don't want to be annoying, but now that it seems like the new release is out (yay!): Would you have time to take a look at this PR? I would really like to know whether it already makes sense to start working on a fully working implementation of this.
Thanks for the ping! I'm a bit deep in the ARM release stuff right now but I did want to make some time to review this stuff. I'll get to it this week as I think I've put this off long enough 😅
Hi,
thanks for the suggestions and answers. I have updated the RFC accordingly.
I was also thinking about renaming the fields of the var-source: aud -> audience ttl -> expiresIn
I think that would make things a little clearer. No need to habe overly short names there.
Done. Added a section with details about the var-source. We should absolutely stick with snake_case, if thats already use everywhere else.
I also fixed a few typos here and there.
@dbaumgarten I made some changes to the RFC, mostly formatting stuff. Give it a look over to check that I didn't miss anything or misrepresent anything you're proposing.
If you're good with it then I'll approve and merge the RFC. You're free to start (or continue I guess) implementing it!
@taylorsilva Looks fine to me :)
Just one small thing: Currently the RFC states the default value vor the audience-setting is "[]".
I think having a default value of nil (which means there will be no aud-claim at all in the token) is a little bit nicer.
If the user really wants an empty array as the value of his aud-claim, he is free to do audience: []
That's fine with me. I went back and read the RFC for JWT's: https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.3
Makes sense to me to provide null instead of an empty array in case it leads to the JWT being rejected by the upstream system. Doesn't read like it would, but I see what you're getting at with suggesting the default value be null/nil.
Great! I am quite amazed how quick and productive the discussion about this RFC was :)
I already made some more progress on the implementation today. I will ping you once it's a fully working prototype. I will probably need some help sooner or later to properly fit this feature into the codebase.