Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug in the encrypt sample, that was causing the decrypt to fail #23816

Merged
merged 7 commits into from
Apr 20, 2021

Conversation

IEvangelist
Copy link
Member

@IEvangelist IEvangelist commented Apr 16, 2021

Summary

  • Upgrade encrypt / decrypt snippets to .NET 5
  • Use top-level statements
  • Use target-type new expressions
  • Use explicitly scoped using to avoid encryption errors

Fixes #23814

Internal preview

✔️ Encrypting data
✔️ Decrypting data

@tdykstra tdykstra requested a review from bartonjs April 19, 2021 17:20
Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I added a request for @bartonjs to review.

@bartonjs
Copy link
Member

The title says that it's fixing a bug in the sample, but there are so many aesthetic changes that I can't see what may have actually needed to be changed.

I strongly recommend using braced usings for cryptographic/security code (and, honestly, for all code, but less strongly). But they already weren't, so ::shrug::.

I'm not sure that the target-typed new enhances readability here. (Personally, I only use it for long lines or multiple-argument generics)

The calls to CreateEncryptor and CreateDecryptor produce IDisposable values that never get disposed (CryptoStream doesn't dispose them). If we're trying for perfection with our IDisposable-usings these'll get pulled out.

And, finally, the snippets lifted into the concept documents no longer match what the samples have. If we want to hand wave and say that they're only highlighting the functional aspects of the code (which is why they're not showing the using statements), that's fine, but the encryption document shows a call to CreateEncryptor(key, iv) when the sample has changed to CreateEncryptor().

No major blockers, but these are my aesthetic observations. It does look like the decrypt and encrypt samples are matched pairs.

@IEvangelist
Copy link
Member Author

IEvangelist commented Apr 20, 2021

Thanks for the review, @bartonjs and @tdykstra.

Yes, that was actually the bug that was being addressed here. I am following your advice now on the explicit scoping of the disposable implementations, part of the issue was not properly scoping these - careless of me really.

I still need someone's approval to merge this one.

Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better 😄

@IEvangelist IEvangelist merged commit dfa6277 into dotnet:main Apr 20, 2021
@IEvangelist IEvangelist deleted the encryption-fixes branch April 20, 2021 02:22
@aynur-safin
Copy link

You have done a great job ). Thank you.

I was late for the party, but maybe it will come in handy.
Encryption uses
encryptWriter.WriteLine("Hello World!");
but when decrypting
await decryptReader.ReadToEndAsync();
the resulting line contains a line break
"Hello World!\r\n"

When decrypting, a single asynchronous call is used
await decryptReader.ReadToEndAsync();
Can be used synchronous to keep everything in the same style, or alternatively make other calls asynchronous
await encryptWriter.WriteLineAsync("Hello World!");
etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encryption example does not work
6 participants