RSACng.Encrypt used wrong "Pkcs1" link.

[CXuesong]

Summary

padding used in RSACng.Encrypt accepts a RSAEncryptionPadding instance, but exception description for CryptographicException used link to RSASignaturePadding. RSACng.Encrypt from RefSrc makes me thinking this exception description should be the same as the one from RSACng.Decrypt.

[mairaw]

Thank you for noticing and fixing the exception issue @CXuesong. I left a suggestion for you to consider before we merge this.

[mairaw]

Suggested change

	<paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>
	<paramref name="padding" />.<see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> isn't equal to <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" /> or <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>

[bartonjs]

I think that's the wrong "or", Maira. The exception is thrown if the padding mode isn't (pkcs1 or oaep), the only two supported modes. I read the "-or-" version as it throws always (throws if != pkcs1, and throws if != oaep... nothing is both, so throws always)

[CXuesong]

I have copied this statement from the same documentation from the Decrypt method, and I think that one should work.

dotnet-api-docs/xml/System.Security.Cryptography/RSACng.xml

Lines 220 to 230 in 056a526

	<returns>The decrypted data.</returns>
	<remarks>To be added.</remarks>
	<exception cref="T:System.ArgumentNullException">
	<paramref name="data" /> is <see langword="null" />.
	-or-
	<paramref name="padding" /> is <see langword="null" />.</exception>
	<exception cref="T:System.Security.Cryptography.CryptographicException">
	<paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>
	</Docs>

[mairaw]

Ok, I'll reword my suggestion. If both conditions should be met, I think we should use and then. Perhaps we might also need to review the Decrypt exception condition @CXuesong.

[mairaw]

@bartonjs can you review my suggestion?

[bartonjs]

<paramref name="padding" />.<see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> isn't equal to <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" /> or <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception> looks reasonable to me.

Except that P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1 should probably be F:System.Security.Cryptography.RSAEncryptionPaddingMode.Pkcs1

[carlossanlop]

@bartonjs is actively documenting these files too. Mentioning him here so he takes a look before merging, @mairaw.

[CXuesong]

Is there any change I need to make before this PR can be accepted? After looking at @mairaw 's & @bartonjs 's comments, I think I may keep current version without changing into "-- or --" version.

[mairaw]

Checking latest comments now...

[CXuesong]

Done modification. In this case, may I also change the exception description in RSACng.Decrypt to keep them consistent?

dotnet-api-docs/xml/System.Security.Cryptography/RSACng.xml

Lines 228 to 230 in 056a526

	<exception cref="T:System.Security.Cryptography.CryptographicException">
	<paramref name="padding" /> does not equal <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Pkcs1" />, or else the <see cref="P:System.Security.Cryptography.RSAEncryptionPadding.Mode" /> of <paramref name="padding" /> does not equal <see cref="F:System.Security.Cryptography.RSAEncryptionPaddingMode.Oaep" />.</exception>
	</Docs>

[carlossanlop]

@CXuesong would you mind rebasing your branch to the latest content in master? @bartonjs has been adding a lot of improvements to the Cryptography API documentation.

@bartonjs would you mind answering @CXuesong 's question?

[bartonjs]

Well, my update of RSA (including this file) is currently in PR, so rebasing should be put off a tiny bit (if I have to make another round of updates I can just take this into mine).

And, yeah, making Decrypt match Encrypt seems good 😄

[bartonjs]

My updates to the RSA classes went in, so rebasing this and incorporating the change for Decrypt are now actionable.

[BillWagner]

ping @carlossanlop @bartonjs

Can one of you help me get the correct status of this one? What is the next action?

stale review