Make showing algo configurable? #20

Activity

1. cjslep commented on Jun 24, 2020

   

   cjslep

   on Jun 24, 2020

   Member

   More actions

   I'm open to it! It could internally be a bool flag. But rather than adding a bool to an existing Signer constructor, let's create a special Signer constructor (ex NewSignerDeprecatedDraft10) that has a comment that documents:

   1. This effective behavior behavior of exposing the algorithm.
   2. That this is based on the old v10 draft of the spec (or older)
   3. To notify API owners that are causing clients to use this NewSignerDeprecatedDraft10 function that they are using an old version of the specification and should update their software to match the specification.

   Let me know if this is something you'd like to contribute; otherwise it'll be something I can get to only when time permits.
2. byrnedo commented on Jun 25, 2020

   

   byrnedo

   on Jun 25, 2020

   Last edited by byrnedo

   Author

   More actions

   I like the idea of the different constructors. I was thinking of some way of doing it in our fork and thought maybe branches at first but this is much better. I can see if I can put something together.

   Actually @cjslep, looking around at the code: wouldn't it be simpler to just have a branch for the draft? Because work involved then is just to reset back to the pre algo hiding commit, push and we're done.

   Across the board bug fixes will be more tedious I guess. But the complexity in general would be lower.
3. cjslep commented on Jun 26, 2020

   

   cjslep

   on Jun 26, 2020

   Member

   More actions

   We might be talking about the same thing, and I probably wasn't being clear!

   If I understand your suggestion of a "branch" to mean "branching statements" in signing.go like the following, I would definitely welcome such mostly-plumbing changes:

   setSignatureHeader(..., showAlgoDeprecatedDraft10 bool) {
     ...
     if (showAlgoDeprecatedDraft10) {
       // Old behavior
     } else {
       // current behavior
     }
   

   where both the macSigner and asymmSigner can call this function like:

   type fooSigner {
     ...
     showAlgoDeprecatedDraft10 bool
   }
   
   func (m *fooSigner) X() {
     setSignatureHeader(..., m.showAlgoDeprecatedDraft10)
   

   And the new NewSignerDeprecatedDraft10 constructor can call newSigner which can now set showAlgoDeprecatedDraft10 to be true on the correct kind of Signer implementation.

   Did I understand you correctly, or is this more work than what you had in mind?
4. byrnedo commented on Jun 26, 2020

   

   byrnedo

   on Jun 26, 2020

   Author

   More actions

   I actually meant a git branch since I thought those kind of conditions could increase complexity.
5. cjslep commented on Jun 26, 2020

   

   cjslep

   on Jun 26, 2020

   Member

   More actions

   Ah I see! Sorry for misunderstanding. I think I'd prefer the code complexity over the organizational complexity of maintaining two git branches.
6. khanzf commented on Jan 8, 2022

   

   khanzf

   on Jan 8, 2022

   More actions

   Hi all, seeing this discussion for the first time, a bit late :) I was about to report the same issue.

   While the specification says to hide the algorithm, in practice most implementations rely on the algorithm name. Thus, using hs2019 confuses the pleroma relay application which consumes the algorithms. The long-term solution is obviously to modify the relay application, but for now I agree that there should be a feature that disables hiding the algorithm.

   I am not clear on what is being said in this reply. Are you saying that this functionality is already being done?
7. khanzf commented on Jan 19, 2022

   

   khanzf

   on Jan 19, 2022

   More actions

   I used the following hand-jam to replace hs20191 with 1rsa-sha2561. The specific reason was because the pleroma relay application uses the algorithm name, so hs2019` fails.

   req.Header["Signature"][0] = strings.Replace(req.Header["Signature"][0], "algorithm=\"hs2019\"", "algorithm=\"rsa-sha256\"", 1)

   In my opinion the specification should be reverted.

   👍React with 👍1Reacted by Gligor Gudjevski

   🎉React with 🎉1Reacted by Gligor Gudjevski