crypto.verifySignature is broken for signatures with expiration

[DmitriyMV]

verifySignature is broken if it tries to retry with the actual verification time. It doesn't reset origText reader, so it has nothing to read on second attempt.

Relevant parts.

[marinthiercelin]

Hey, thanks for opening the issue.
That seems indeed to be broken.
I'm a bit confused why you would revert your project to v2.5.2 though, the issue seems older than this version.
Do you have any example data that I could use to trigger the bug ?

[DmitriyMV]

I'm a bit confused why you would revert your project to v2.5.2 though, the issue seems older than this version.

verifySignature is indeed broken in 2.5.2 (it doesn't return an error if signature expired) but we are not hitting it, because we have check called IsExpired which is doing what it must. So that's a reason for revert.

Do you have any example data that I could use to trigger the bug ?

Sure, here it is.

[DmitriyMV]

IIUC there are two approaches to solves this, without changing the external API.

1. Change verifySignature origText from io.Reader to func() io.Reader. That way we can ensure that it always get the new reader. the downside - it will look a bit strange, and you have to keep in mind that origText func() need to return new reader each time. Also VerifyDetachedStream accepts a reader so I dunno what can be done there.
2. Check if origText supports Seek. If does - good! If not - read it into the local bytes.Buffer and use it instead. Downside - for io.Readers who do not support Seek you will need to read entire message before processing it.

Or change origText to ReadSeeker but that will break KeyRing.VerifyDetachedStream.

I can prepare a PR for any of those fixes if you wish.

[marinthiercelin]

Hey, It's indeed tricky to fix while keeping the API.

I think, as a short term fix, we should go with a variant of your second approach.
If origText supports Seek use it and verify again.
But if it does not support Seek, I don't think we should read it all in a buffer, that could be quite bad for memory usage.
Instead I think we should just not reattempt the verification.
That means that if you verify with VerifyDetachedStream with a non seekable stream, the verification might fail.
So it's only a partial fix.

For a complete fix, I think we should instead rethink how we do the verification, and manually check the dates for expiration, rather than reattempting verification with a different time.

If you want to open a PR for either the partial fix, or look into the complete fix, it would be greatly appreciated.

[DmitriyMV]

Opened an PR for partial fix.

[marinthiercelin]

Thanks a lot for your submission @DmitriyMV

[smira]

@DmitriyMV is there something else which needs to fixed here? as it seems that the verification is still broken?

[DmitriyMV]

@smira It's a partial fix in a sense that it only supports messages which have Seek method. It may still trigger for non Seek-able streams, but for the most part it is good enough.