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

add various const specifiers to pk utils #1194

Closed
wants to merge 1 commit into from

Conversation

bl4ck5un
Copy link

@bl4ck5un bl4ck5un commented Dec 1, 2017

This allows const pointers to secret keys in functions such as mbedtls_ssl_conf_own_cert. Related to this forum discussion. Also related to RP #1004.

Notes:

@RonEld
Copy link
Contributor

RonEld commented Dec 4, 2017

Hi @bl4ck5un Thank you for your contribution!
Note that since this is a change in API, so it won't be inserted until there will be a major release, assuming we agree to adopt this change.
In addition, unfortunately we cannot accept contributions without a signed CLA. Could you please refer me to the CLA you have signed?

@bl4ck5un
Copy link
Author

bl4ck5un commented Dec 4, 2017

I've signed this contributor_agreement. I signed under username v0ido9.

@RonEld
Copy link
Contributor

RonEld commented Dec 4, 2017

Thank you for your confirmation

@mazimkhan mazimkhan self-assigned this Mar 21, 2018
@mazimkhan
Copy link

retest

@mazimkhan
Copy link

@bl4ck5un can you please rebase you PR to development head. There have been some fixes in development branch to stabilise the timing tests.

@bl4ck5un
Copy link
Author

@mazimkhan rebased my commit on f3ada4a

@simonbutcher
Copy link
Contributor

Hi @bl4ck5un,

We've discussed adoption of this PR, and whilst it makes sense, because it introduces const across the API, it's difficult for us to merge as it would break source level compatibility with our existing API for users already using the library. That doesn't mean we don't want to make these changes, just that the timing should be right, and we should do it in a big API breaking release. Maybe Mbed TLS 3.0.

As such, we're not going to merge this PR soon, and by the time we may get around to it, the PR may be outdated.

Therefore, @k-stachowiak has raised this as issue #1485, which records this PR, and when we get to a point in the lifecycle of the library where we can make these changes, we'll revisit this, and may use this PR.

Rather than keep this PR open indefinitely, I prefer to close it now, and revisit this area at a later time when we look at #1485.

Thanks for your support of the project and this contribution. We do want to adopt these ideas and changes, but as I say, the timing has to be right.

@mpg
Copy link
Contributor

mpg commented Feb 1, 2021

While I tend to agree with the part that adds const for key writing functions, I have more mixed feelings about adding const to things like pk_sign(): actually, pk_sign() can modify the state of the private key context in some cases (for example, updating blinding values for RSA).

Now technically the const would still be correct, as in C const is not "deep": it only means we don't modify the members of the structure, and we're allowed to modify the objects pointed to by members of the structure (which again, at least pk_sign() does, at least with RSA), but "morally" I'm not sure I feel comfortable adding const here as I'm afraid it might give the wrong impression. This impression could matter when it comes to questions such as "can I make concurrent calls to pk_sign() from various threads using the same pk_context?" Currently the answer to that question (for RSA) is "not unless you built Mbed TLS with MBEDTLS_THREADING_C enabled (which it isn't by default)". Making the function accept a const pk_context * might make it look like the answer is yes, which it isn't (in the default build).

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.

5 participants