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

[iOS] DSA API surface on iOS #52758

Closed
filipnavara opened this issue May 14, 2021 · 6 comments · Fixed by #52978
Closed

[iOS] DSA API surface on iOS #52758

filipnavara opened this issue May 14, 2021 · 6 comments · Fixed by #52978
Labels
area-System.Security os-ios Apple iOS untriaged New issue has not been triaged by the area owner

Comments

@filipnavara
Copy link
Member

Ref: #51926 (comment), #52755 (comment)

Currently System.Security.Cryptography.DSA.Create returns a useless object on iOS. The object doesn't implement key generation, signing or verification since none of them are supported at the OS level. This is obviously not ideal because it may falsely lead someone to believe that the DSA works under some circumstances and it increases the binary size.

There are two way to deal with it:

  • Leave it as is and annotate the APIs with appropriate attributes to give people compile-time warnings from the platform analyzer.
  • Remove DSASecurityTransforms on iOS completely and make DSA.Create itself throw PlatformNotSupportedException. This has some collateral damage where CryptoConfig needs to be modified not to return any DSA types to avoid breaking assumptions elsewhere in the code.

Preferably this should be decided and cleaned up before putting any more annotations in the S.S.C.Algorithms code and before specific DSA-related tests are disabled.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels May 14, 2021
@ghost
Copy link

ghost commented May 14, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Ref: #51926 (comment), #52755 (comment)

Currently System.Security.Cryptography.DSA.Create returns a useless object on iOS. The object doesn't implement key generation, signing or verification since none of them are supported at the OS level. This is obviously not ideal because it may falsely lead someone to believe that the DSA works under some circumstances and it increases the binary size.

There are two way to deal with it:

  • Leave it as is and annotate the APIs with appropriate attributes to give people compile-time warnings from the platform analyzer.
  • Remove DSASecurityTransforms on iOS completely and make DSA.Create itself throw PlatformNotSupportedException. This has some collateral damage where CryptoConfig needs to be modified not to return any DSA types to avoid breaking assumptions elsewhere in the code.

Preferably this should be decided and cleaned up before putting any more annotations in the S.S.C.Algorithms code and before specific DSA-related tests are disabled.

Author: filipnavara
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@steveisok
Copy link
Member

I think PNSE on DSA.Create seems like the cleaner approach if it's possible to pull off.

@filipnavara
Copy link
Member Author

It's definitely possible. At some point I had a branch with it but I decided not to pursue it as part of the bring-up PR because it would be hard to read along with the other changes.

@filipnavara
Copy link
Member Author

I'll revive the branch once my other iOS PRs are merged, it would only create conflicts now.

@bartonjs
Copy link
Member

I think PNSE on DSA.Create seems like the cleaner approach if it's possible to pull off.

I agree. There may be some annoying collateral, but it's probably OK.

private AsymmetricAlgorithm LoadAnything(ReadOnlySpan<byte> data)
{
    RSA rsa = RSA.Create();
    DSA dsa = DSA.Create();
    ECDsa ecdsa = ECDsa.Create();

    try
    {
        rsa.ImportRSAPrivateKey(data, out _);
        return rsa;
    }

// repeat for all file formats of all types
}

This kind of thing would need to structure the code to not call DSA.Create too early (and to check for it failing). But it'll be pretty rare, I think, and the platform squiggler will warn them if they say iOS is in scope.

@marek-safar marek-safar added the os-ios Apple iOS label May 14, 2021
@filipnavara
Copy link
Member Author

The change is quite small: https://github.com/dotnet/runtime/compare/main...filipnavara:dsa-ios?expand=1 ... it can be cleaned up a bit once #52191 is merged to remove DSASecurityTransforms.iOS.cs completely.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 19, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 21, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security os-ios Apple iOS untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants