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

get encoder library from smaller repo, avoid silently failing if enco… #131

Merged
merged 6 commits into from
Apr 6, 2023

Conversation

bhess
Copy link
Member

@bhess bhess commented Mar 25, 2023

  • Gets encoder library from smaller repository
  • Avoid encoding tests silently fail if algorithm/encoding is not found
  • Points to new upstream encoder version with updated OID
  • Adds UPDATE_DISCONNECTED 1 to allow offline build (Re-enable offline build #139)

Fixes #129
Fixes #130

@bhess bhess marked this pull request as ready for review March 25, 2023 16:48
@bhess bhess requested a review from baentsch as a code owner March 25, 2023 16:48
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. But I fail to see a behavioural change in the code, so am I assuming right that it will fail in the same way if the OIDs between qsc-encoding lib and oqs-provider go out of sync again? Also, wouldn't it be more professional to have tests for this (both good and negative expected behaviour)? Finally, is such duplication of OIDs between oqs-provider and qsc-encoder really necessary (or at the very least cannot be overruled at the API level)? It creates an undue dependency that we've just fallen into -- and can fall into again if I read Quantum-Safe-Collaboration/qsc-key-encoder@d7970ea correctly.

It is understood this is a quick fix to help Goutam this weekend, but I'd like to bring this PR to a proper completion.

@xvzcf: Please take this branch as a hotfix if you can still need it in tomorrows testing session. I also published a docker image at openquantumsafe/oqs-ossl3:ietf116-hotfix[-dev] to make your life easier.

oqsprov/oqsprov_keys.c Outdated Show resolved Hide resolved
@bhess
Copy link
Member Author

bhess commented Mar 25, 2023

Thanks for the fixes. But I fail to see a behavioural change in the code, so am I assuming right that it will fail in the same way if the OIDs between qsc-encoding lib and oqs-provider go out of sync again? Also, wouldn't it be more professional to have tests for this (both good and negative expected behaviour)?

It will not silently fail anymore. Good point with the test, I can add a negative test (show that the tests fail if a unknown encoding is set via env var).

Finally, is such duplication of OIDs between oqs-provider and qsc-encoder really necessary (or at the very least cannot be overruled at the API level)? It creates an undue dependency that we've just fallen into -- and can fall into again if I read Quantum-Safe-Collaboration/qsc-key-encoder@d7970ea correctly.

I'm not sure what's the best solution. How would you overrule at API level?
There needs to be a unique key to match. Another way the API supports is lookup by algorithm name. Here the issue is that the names are not really well defined, e.g, OQS name: SPHINCS+-SHAKE256-128s-robust and the name used in the reference implementation: sphincs-shake256-128s-robust. The names also don't identify the algorithm version. OIDs seem to define things more uniquely (ideally there will be a OID repo to pull from). The Falcon OID change was unrelated to key encoding (but signatures changed). Next time the key encoding could change which would require a new IETF draft revision.

Thanks for preparing the docker hotfix!

@baentsch
Copy link
Member

baentsch commented Mar 26, 2023

How would you overrule at API level?

Client (oqs-provider) requests algorithm name (of qsc-encoder) w/o OID reference: It should be the responsibility of the client that algorithm name and OID match (or not, e.g., at an interop hackathon where people play with OIDs).

There needs to be a unique key to match.

Only when there are different encodings for the same algorithm name, right? Are there already different encoding for the same algorithm (name)? If it were coming to that, what about a SHA(256) over an (agreed upon) KAT to identify algs with the same name (but different versions/KATs)?

names are not really well defined, e.g, OQS name: SPHINCS+-SHAKE256-128s-robust and the name used in the reference implementation: sphincs-shake256-128s-robust

"There's no problem in computer science that cannot be solved by ... a lookup table" (or so :) You could leave that to the client -- just document the naming approach (and stick to it) in the encoder.

@bhess
Copy link
Member Author

bhess commented Mar 28, 2023

Hm ok, just using names might indeed be the better option given how OIDs are in flux (and configurable via env vars).

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks for explanations and update. I presume testing (good/bad cases) and documentation for these changes (env vars) will be added in further commits to this PR (?)

@bhess
Copy link
Member Author

bhess commented Mar 30, 2023

Thanks for explanations and update. I presume testing (good/bad cases) and documentation for these changes (env vars) will be added in further commits to this PR (?)

Added a commit with both.

@baentsch
Copy link
Member

Added a commit with both.

Thanks. Then what am I doing wrong here (after the tests ran, using the resultant .crt files):

As expected:

> OPENSSL_MODULES=_build/lib LD_LIBRARY_PATH=.local/lib64 .local/bin/openssl x509 -in tmp/falcon1024.crt -text -noout -provider oqsprovider -provider default
[...]
        Subject: CN=test Server
        Subject Public Key Info:
            Public Key Algorithm: falcon1024
                falcon1024 public key:
                PQ key material:
[...]

Not quite expected:

> OPENSSL_MODULES=_build/lib LD_LIBRARY_PATH=.local/lib64 .local/bin/openssl x509 -in tmp/falcon512.crt -text -noout -provider oqsprovider -provider default
[...]
        Subject: CN=test Server
        Subject Public Key Info:
            Public Key Algorithm: falcon512
            Unable to load Public Key
808B1AEC977F0000:error:03000072:digital envelope routines:X509_PUBKEY_get0:decode error:crypto/x509/x_pubkey.c:463:
808B1AEC977F0000:error:03000072:digital envelope routines:X509_PUBKEY_get0:decode error:crypto/x509/x_pubkey.c:463:
        X509v3 extensions:
[...]

Trying with

OPENSSL_MODULES=_build/lib OQS_ENCODING_FALCON512=draft-uni-qsckeys-falcon-00/sk-pk OQS_ENCODING_FALCON512_ALGNAME=Falcon512 LD_LIBRARY_PATH=.local/lib64 .local/bin/openssl x509 -in tmp/falcon512.crt -text -noout -provider oqsprovider -provider default

yields the same result.

What did I do? Changed the Falcon512 OID. --> Is there still some dependency on it?

@bhess
Copy link
Member Author

bhess commented Mar 30, 2023

I could reproduce the error, however, I get an error even without changing the OID (Falcon1024 looks fine, Falcon512 shows the error). I will need to investigate why. There shouldn't be any OID dependency.

(another issue described earlier here wasn't one)

@bhess
Copy link
Member Author

bhess commented Apr 3, 2023

I could reproduce the error, however, I get an error even without changing the OID (Falcon1024 looks fine, Falcon512 shows the error). I will need to investigate why. There shouldn't be any OID dependency.

I didn't set all env variables correctly when testing this. Trying again, it seems to all work ok, including changing the OID.

Testing Falcon with changed OIDs:

export OQS_ENCODING_FALCON512=draft-uni-qsckeys-falcon-00/sk-pk
export OQS_ENCODING_FALCON1024=draft-uni-qsckeys-falcon-00/sk-pk
export OQS_ENCODING_FALCON512_ALGNAME=Falcon512
export OQS_ENCODING_FALCON1024_ALGNAME=Falcon1024
export OQS_OID_FALCON512=1.3.9999.3.20
export OQS_OID_FALCON1024=1.3.9999.3.21
  • run scripts/runtests_encodings.sh

  • test Falcon1024

$ OPENSSL_MODULES=_build/lib LD_LIBRARY_PATH=.local/lib64 .local/bin/openssl x509 -in tmp/falcon1024.crt -text -noout -provider oqsprovider -provider default
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            52:1a:85:d8:b7:66:c8:0f:f6:86:07:4a:eb:24:6c:c9:55:94:8b:7d
        Signature Algorithm: falcon1024
        Issuer: CN=test CA
        Validity
            Not Before: Jan 23 08:00:00 2015 GMT
            Not After : Aug 23 09:00:00 2025 GMT
        Subject: CN=test Server
        Subject Public Key Info:
            Public Key Algorithm: falcon1024
                falcon1024 public key:
                PQ key material:
                    0a:70:9a:b5:96:8c:e7:e3:80:ae:9b:80:3c:21:c0:
  • test Falcon512
% OPENSSL_MODULES=_build/lib LD_LIBRARY_PATH=.local/lib64 .local/bin/openssl x509 -in tmp/falcon512.crt -text -noout -provider oqsprovider -provider default 
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            13:b7:60:75:a4:0e:99:d8:a9:6f:07:61:5d:7d:e1:aa:05:9a:1f:15
        Signature Algorithm: falcon512
        Issuer: CN=test CA
        Validity
            Not Before: Jan 23 08:00:00 2015 GMT
            Not After : Aug 23 09:00:00 2025 GMT
        Subject: CN=test Server
        Subject Public Key Info:
            Public Key Algorithm: falcon512
                falcon512 public key:
                PQ key material:
                    09:b4:d2:f4:28:df:03:a1:42:3d:89:c9:a4:26:4c:

After unset OQS_OID_FALCON512, the above test fails as expected with a decoding error.

What did I do? Changed the Falcon512 OID. --> Is there still some dependency on it?

@baentsch looking at your test command a OQS_OID_FALCON512=... might be missing.

@bhess bhess mentioned this pull request Apr 3, 2023
@baentsch
Copy link
Member

baentsch commented Apr 3, 2023

@baentsch looking at your test command a OQS_OID_FALCON512=... might be missing.

Nope. I changed generate.yml for the test...

@bhess
Copy link
Member Author

bhess commented Apr 3, 2023

Fixed a bug in 1089c76 that checked a wrong index, with the effect that the encoding was not always applied (if env vars for other/unrelated algorithms were not set).

Nope. I changed generate.yml for the test...

Checked this case as well, seems to work now with the fix above.

@baentsch
Copy link
Member

baentsch commented Apr 3, 2023

Thanks for these fixes. I hope you can forgive me now being a "bit nervous" about the robustness of all this -- my question thus: Does IETF-Hackathon/pqc-certificates#43 contain proper (proper in the sense of "in line with your RFC") certificates? Or should this data set be re-done after landing this PR to permit others to test "the right stuff"? Last question: Are you going to rebase the PR to remove the conflict?

@bhess
Copy link
Member Author

bhess commented Apr 3, 2023

Thanks for these fixes. I hope you can forgive me now being a "bit nervous" about the robustness of all this -- my question thus: Does IETF-Hackathon/pqc-certificates#43 contain proper (proper in the sense of "in line with your RFC") certificates? Or should this data set be re-done after landing this PR to permit others to test "the right stuff"?

The certs produced with the "hotfix" docker image are equivalent with the ones now in this PR. Thanks for being extra cautious!

Last question: Are you going to rebase the PR to remove the conflict?

Done.

@baentsch
Copy link
Member

baentsch commented Apr 3, 2023

The certs produced with the "hotfix" docker image are equivalent with the ones now in this PR.

Thanks for checking (and re-basing).

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks for the various fixes, @bhess: All works as desired. And in one instance better than I thought possible: After changing an OID via env var during cert gen, it is even possible to read certs generated that way without explicitly setting the changed OID as env var also to the openssl x509 -text command. Hmm. Would you know why this works? Some redundancy in the SPKI info?

@baentsch baentsch merged commit 1ab7171 into main Apr 6, 2023
@baentsch baentsch deleted the bhe-encimprov branch April 6, 2023 05:16
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 16, 2024
open-quantum-safe#131)

* get encoder library from smaller repo, avoid silently failing if encoder/algorithm not found

* Call encoder algorithms by name, configurable via env vars.

* add negative tests, add docs

* UPDATE_DISCONNECTED

* fix encoding lookup

Signed-off-by: Felipe Ventura <felipe.ventura@entrust.com>
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.

Improve qsc encoder integration qsc_encodings silently failing
2 participants