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.

Sign up for GitHub

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

Jump to bottom

build: add apisupport low_level_rsa #636

Merged
merged 1 commit into from
Feb 16, 2025
Merged

build: add apisupport low_level_rsa #636

merged 1 commit into from
Feb 16, 2025

Conversation

gperciva
Copy link
Member

@gperciva gperciva commented Feb 9, 2025

No description provided.

@gperciva
Copy link
Member Author

gperciva commented Feb 9, 2025

I'm not very happy about this PR; opening for discussison.

OpenSSL 3.0 marked many RSA functions as "deprecated", so this adds an apisupport-LIBCRYPTO-LOW_LEVEL_RSA (just like we did for AES).

The 4 files which need that flag are now being compiled into a new libtarsnap_rsa.a.

However, crypto_keys_init() (which is one of those 4 files) requires crypto_file_init_keys(). In order to resolve symbols, I've included 3 other files in this library. (This is what I'm most unhappy about.)

Also, this isn't an air-tight dependency resolution... crypto_file need numerous other symbols such as crypto_aes. I'm guessing that those are resolved because libtarsnap.a already needed them, so the linker already included them in the binary.

I see a few possibilities:

  1. Current PR: have a library for those 4 files, plus the 3 others that are required.
  2. Have a library for only those 4 files, then add libraries to the linker command-line multiple times. (However, this needs more than 2 instances of libtarsnap.a.)
  3. Give up on making openssl happy with our use of RSA. If somebody tries to compile tarsnap and it fails due to deprecation reports, then we tell them to manually add -Wno-deprecated-declarations to their CFLAGS. (This is the current situation.)

@cperciva
Copy link
Member

Can we just yank crypto_keys_init into a new lib/crypto/crypto_keys_init.c file?

@cperciva
Copy link
Member

Something like #637

@gperciva
Copy link
Member Author

... huh, I even wrote "refactor lib/crypto/ as option 4, but then deleted it because I figured there was no way you'd go for it. I thought it would need a much more invasive refactoring.

That solves this, and allows libtarsnap_rsa.a to contain only the 4 files that really needs to.

I'm not positive about the name, but that's the only remaining quibble. Another option would be libtarsnap_libcrypto_rsa, or even libtarsnap_crypto_rsa.a.

(I think the latter is too-easily confused with crypto_rsa.c, but evidently I shouldn't self-censor ideas.)

@gperciva
build: add apisupport low_level_rsa
d3d4bbf 
OpenSSL 3.0 marked many RSA functions as "deprecated"; tarsnap uses some
of them[1] in:
    lib/crypto/crypto_compat.c
    lib/crypto/crypto_keys.c
    lib/crypto/crypto_keys_subr.c
    lib/crypto/crypto_rsa.c

Those files are now being compiled into a new libtarsnap_rsa.a, which
uses the appropriate compiler flags.

[1] List of relevant deprecated OpenSSL 3.0 RSA functions:
    RSAPublicKey_dup
    RSA_bits
    RSA_free
    RSA_generate_key_ex
    RSA_get0_crt_params
    RSA_get0_factors
    RSA_get0_key
    RSA_new
    RSA_private_decrypt
    RSA_private_encrypt
    RSA_public_decrypt
    RSA_public_encrypt
    RSA_set0_crt_params
    RSA_set0_factors
    RSA_set0_key
    RSA_size
@gperciva gperciva marked this pull request as ready for review February 16, 2025 18:05
@gperciva
Copy link
Member Author

Working, rebased, ready for review.

I still have some minor qualms about the name of the library, but that's it.

@cperciva cperciva merged commit 8ce2a7a into master Feb 16, 2025
2 checks passed
@gperciva gperciva deleted the build-openssl-rsa branch February 16, 2025 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gperciva @cperciva