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

[WIP] Add RedHat keyfile-corpus as an external test #15188

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jon-oracle
Copy link
Contributor

Check that we can read/decode each of the PKCS#12 files in the repo:

https://github.com/redhat-qe-security/keyfile-corpus

The tests run as part of the external test suite. Keyfile-corpus is added as a git submodule which needs to be init'ed before the tests can be run (external test system does this).

Currently there are failures due to:

  • RC2 AI encoding issue Some old algorithms in PKCS#12 files don't work any more #15070
  • A failure in a single file (keyfile-corpus/rsa(2048,sha256),cert&key(PBES2(PBKDF2(salt(8),iter(2048),keyLen(default),prf(default)),aes-128-cbc(IV(16)))),mac(sha1,salt(8),iter(2048)),pass(unicode,openssl-1.0.2k-1.fc24).p12), to be investigated.

Also, the tests are not yet comprehensive. They do not:

  • Check any contents (e.g. certificates) against the reference data
  • Decrypt and check private keys

The above would require either additions to the pkcs12 app or some other means of reading/verifying the files.

  • tests are added or updated

@t8m
Copy link
Member

t8m commented May 7, 2021

The above would require either additions to the pkcs12 app or some other means of reading/verifying the files.

Not sure what's missing in the pkcs12 app. You would just convert the pkcs12 file into the pem file containing certs and keys and then you would just try to find the reference data in the output. Wouldn't that be sufficient?

@t8m
Copy link
Member

t8m commented May 7, 2021

Although it is not a requirement for this PR and just trying to parse the pkcs12 files is also useful as a test.

@jon-oracle
Copy link
Contributor Author

The above would require either additions to the pkcs12 app or some other means of reading/verifying the files.

Not sure what's missing in the pkcs12 app. You would just convert the pkcs12 file into the pem file containing certs and keys and then you would just try to find the reference data in the output. Wouldn't that be sufficient?

I'm not sure there's currently a way to provide separate passwords, e.g. the PKCS#8 key password or separate mac/encrypt passwords using just the command line without getting a prompt for user input. Having '-encpass', '-keypass' and '-macpass' options might be sufficient. I don't think we can cover all possible combinations allowed by PKCS#12 though using just the command line, e.g. separate passwords for each encrypted key.

For testing, splitting the file up before processing/decrypting the parts is an option though. I'll take a look at this approach.

@slontis
Copy link
Member

slontis commented May 25, 2021

Why do you skip the malformed files.. Could they be added as negative tests?

@p12_files = grep(!/pass-cipher/, @p12_files);

# Skip files with an empty password - pkcs12 app cannot handle this
@p12_files = grep(!/pass\(empty/, @p12_files);
Copy link
Contributor

Choose a reason for hiding this comment

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

it can handle them, use password-newline.txt created like so: echo > password-newline.txt as password file

Comment on lines +47 to +55
ok(run(app(["openssl", "pkcs12",
"-noout",
#"-info",
"-in", "kf-$fnum.p12",
"-provider-path=$provider_path",
"-provider", "legacy",
"-provider", "default",
#"-nokeys", NOTE: pkcs12 app does not allow key password as an argument, yet.
"-password", "file:$passfile"])),
Copy link
Contributor

Choose a reason for hiding this comment

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

I've got good experience by using -info (easier debugging) and -out /dev/null -nodes (as some issues didn't manifest themselves without actually trying to re-export the keys) instead of -noout

@tomato42
Copy link
Contributor

tomato42 commented Aug 5, 2021

I've just released version 0.3.0 with files that use SHA512/224, SHA512/256 and SHA-3 in PRF and MAC

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Aug 6, 2021
@t8m t8m added this to the Post 3.0.0 milestone Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants