Skip to content

Add a allow_export flag to restrict exporting #466

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

Merged
merged 4 commits into from
Jul 13, 2021

Conversation

hug-dev
Copy link
Member

@hug-dev hug-dev commented Jul 8, 2021

On some PKCS11 implementations, it is not allowed to create a key that
can be exported. For that reason, prevent creating exportable keys from
the configuration file so that clients can have a better return value.

This probably should be written in the book as well, as a new possible return value for generate key.

Fix #462

@hug-dev hug-dev added this to the Parsec Release 0.8.0 milestone Jul 8, 2021
@hug-dev hug-dev requested a review from a team as a code owner July 8, 2021 15:00
@hug-dev hug-dev requested a review from ionut-arm July 12, 2021 10:08
@ionut-arm
Copy link
Member

But you're not exactly solving the problem in #462 - you're still setting the attributes for keys that have export as false.

@hug-dev
Copy link
Member Author

hug-dev commented Jul 12, 2021

As per the investigation in here, I checked that those attributes are actually allowed (in Layerscape) but they can't be set to unsecure mode (exportable=true, sensitive=false). It would still work if they explicitely set to the secure mode. That's why I "simplified" what we planned to this.
I believe Yubico will follow the same logic (as per this)

@ionut-arm
Copy link
Member

Oh, it's because we have a bug 😬 I was wondering wth is going on, because what you're saying and what Sahil reported are at odds - most keys we generate for tests have export set to false, so it made no sense that create_and_destroy would fail if what you said above were true.

However, see if you can spot something wrong with this:

priv_template.push(Attribute::Sensitive((usage_flags.export).into()));

@wiktor-k
Copy link

Folks, if we only patch the entire world of buggy PKCS#11 drivers then this PR could be obsoleted! 🥳

@ionut-arm
Copy link
Member

Folks, if we only patch the entire world of buggy PKCS#11 drivers then this PR could be obsoleted! 🥳

Unfortunately not, see my comment above ☹️ we set SENSITIVE and EXTRACTABLE to the same boolean value, when they should be "opposites"

@wiktor-k
Copy link

Oh, right. That's totally something to improve. And what a coincidence that buggy PKCS#11 drivers exposed a bug in parsec :) Fortunately this one is easy to fix.

@hug-dev
Copy link
Member Author

hug-dev commented Jul 12, 2021

Should I add the fix in this PR?

Would have been funny if you did not spot the inverted logic and the double bug in Parsec and there made the key secure

@ionut-arm
Copy link
Member

Should I add the fix in this PR?

I think so - it's only one line and related to what's going on in here (and it was discovered via the same conversation).

@wiktor-k
Copy link

Continuous Integration / Integration tests using Cypto Trusted Service provider

I think there's a typo in the test name that failed ^^ :)

hug-dev added 3 commits July 13, 2021 10:04
On some PKCS11 implementations, it is not allowed to create a key that
can be exported. For that reason, prevent creating exportable keys from
the configuration file so that clients can have a better return value.

Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

👍🏻

Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
@hug-dev hug-dev merged commit 0117c48 into parallaxsecond:main Jul 13, 2021
@hug-dev hug-dev deleted the allow-export branch July 13, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PKCS11 provider export-attributes switch
3 participants