mattosaurus / PgpCore

.NET Core class library for using PGP
MIT License
245 stars 98 forks source link

Add ability to extract clear-signed content when verifying signature #148

Closed RodneyRichardson closed 2 years ago

RodneyRichardson commented 2 years ago

I've added additional versions of VerifyClearArmoredString+Async to output the content as part of performing verification.

I've also remove an unused variable (outputStream) in the VerifyArmoredString methods (which admittedly could have been a separate commit).

This (at least partially) addresses #77.

RodneyRichardson commented 2 years ago

I wasn't sure what the purpose of LegacyXX tests were, so I've just added tests to match what was there already.

RodneyRichardson commented 2 years ago

I wasn't sure about returning a tuple from the Async methods. Would it be better to define a structure, and return that? I'm unfamiliar with how other .NET languages (e.g. VB) handle tuples.

mattosaurus commented 2 years ago

Hi, thanks for submitting the PR. I'm currently on holiday for the next 10 days but will take a look over this when I get a chance.

The legacy tests are because the way keys are read changed a couple of yers ago but I wanted to keep the old method of supplying them as well. Thanks for creating both versions of the tests.

RodneyRichardson commented 2 years ago

@mattosaurus I've just updated this from master, so it should be ready for review now.

mattosaurus commented 2 years ago

Thanks, this looks good though I'm not really a fan of returning a Tuple from the VerifyAndReadClearArmoredString method. Would you have any objection to removing this and just keeping the version using the out parameter?

RodneyRichardson commented 2 years ago

Thanks, this looks good though I'm not really a fan of returning a Tuple from the VerifyAndReadClearArmoredString method. Would you have any objection to removing this and just keeping the version using the out parameter?

That's fine. I'm not a fan either (too pythonic!) :)

Edit: Actually, I remember now why I did it that way - async methods cannot have out parameters. The VerifyAndReadClearArmoredString methods don't return a tuple, only the VerifyAndReadClearArmoredStringAsync methods.

Options:

  1. Leave it as it is.
  2. Create a VerificationResult struct (or similar name), and return that instead of the tuple
  3. Remove the Async methods.

If we went for option 2, I'd probably want to use it as the return value for both sync and async methods.

I'm happy to do any of these options - let me know your preference. I think Option 2 is my preference.

RodneyRichardson commented 2 years ago

I have gone with Option 2.

mattosaurus commented 2 years ago

Sorry for the delay, I agree that option 2 is the best option. I'll take a look over this in the next few days.

Thanks for putting in the time to add this feature.

RodneyRichardson commented 2 years ago

Thanks for putting in the time to add this feature.

No worries. Thanks for creating the library!