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

[Android] Split android_security.c into multiple files #44603

Merged
merged 2 commits into from
Nov 13, 2020

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 12, 2020

Following the OpenSSL and AppleTLS PAL conventions.
Aslo, I added pal_bignum implementation (based on java.math.BigInteger) - it's needed for upcoming RSA impl (and other APIs).

@ghost
Copy link

ghost commented Nov 12, 2020

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


Issue meta data
Issue content:
Following the OpenSSL and AppleTLS PAL convention.

Aslo, I added pal_bignum implementation (based on java.math.BigInteger) - it's needed for upcoming RSA impl (and other APIs).

</td>
Issue author: EgorBo
Assignees: -
Labels:
`area-System.Security`

</td>
Milestone: -

@@ -55,7 +55,7 @@
<Exec Condition="'$(DeployAndRun)' == 'true'" Command="$(AdbTool) install $(ApkDir)/bin/HelloAndroid.apk" />
<Exec Condition="'$(DeployAndRun)' == 'true' and '$(RunActivity)' != 'true'" Command="$(AdbTool) shell am instrument -w net.dot.HelloAndroid/net.dot.MonoRunner" />
<Exec Condition="'$(DeployAndRun)' == 'true' and '$(RunActivity)' == 'true'" Command="$(AdbTool) shell am start -n net.dot.HelloAndroid/net.dot.MainActivity" />
<Exec Condition="'$(DeployAndRun)' == 'true'" Command="$(AdbTool) logcat -d -s DOTNET" />
<Exec Condition="'$(DeployAndRun)' == 'true'" Command="$(AdbTool) logcat -d" />
</Target>
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: -s DOTNET hides asserts and since we use samples as development playgrounds we don't need it.

PALEXPORT int32_t CryptoNative_EvpDigestUpdate(jobject ctx, void* d, int32_t cnt);
PALEXPORT int32_t CryptoNative_EvpDigestFinalEx(jobject ctx, uint8_t* md, uint32_t* s);
PALEXPORT int32_t CryptoNative_EvpDigestCurrent(jobject ctx, uint8_t* md, uint32_t* s);
PALEXPORT void CryptoNative_EvpMdCtxDestroy(jobject ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Unless there's a goal that the Android shim be able to be drop-in substituted for the OpenSSL shim (which feels like a hard state to maintain), the functions in this library should be named more appropriate to their context.

  • CryptoNative is OK as long as this library and the OpenSSL shim can't ever be loaded in the same process, otherwise the exported function prefix for this library should be different (e.g. AndroidCryptoNative).
  • Names like EvpDigestUpdate are because it's a thin wrapper over EVP_DigestUpdate. Since the Android implementation doesn't involve a function of that name the name should be more reflective of what is going on.

So I'd expect CryptoNative_EvpDigestUpdate to be AndroidCryptoNative_MessageDigestUpdate

Copy link
Member Author

Choose a reason for hiding this comment

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

@bartonjs thats the plan, but I'd like to do it a bit later when it will be clear that we can replace openssl.

@EgorBo EgorBo merged commit ce1d60f into dotnet:master Nov 13, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants