Make showing algo configurable?

[byrnedo]

This commit 221cfc4
hides the algo from the receiver.

I'm integrating against apis that don't expect this.
Are we open for making this behaviour configurable?

[cjslep]

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.

[byrnedo]

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.

[cjslep]

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?

[byrnedo]

I actually meant a git branch since I thought those kind of conditions could increase complexity.

[cjslep]

Ah I see! Sorry for misunderstanding. I think I'd prefer the code complexity over the organizational complexity of maintaining two git branches.

[khanzf]

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?

[khanzf]

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.