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

Drop internal SHA1 implementation #8034

Merged
merged 5 commits into from
Dec 12, 2022
Merged

Conversation

locker
Copy link
Member

@locker locker commented Dec 9, 2022

Historically, we use our internal SHA1 implementation instead of the one provided by OpenSSL. The reason for that is that initially we didn't link OpenSSL. Instead, we loaded it dynamically on demand. We targeted to work even on those distributions where the OpenSSL library isn't installed, see #405. Those days are long gone. Now, we link the OpenSSL library unconditionally, see #1382. Let's drop our internal SHA1 implementation in favor of OpenSSL's.

While we are at it, let's also merge the scramble library into the 'chap-sha1' authentication method implementation. There's no need in it anymore, because the authentication methods now provide a clean API, see #7986.

Closes #7987

@coveralls
Copy link

coveralls commented Dec 9, 2022

Coverage Status

Coverage decreased (-0.02%) to 84.831% when pulling 66f6ec2 on locker:drop-internal-sha1 into a392eb7
on tarantool:master
.

@alyapunov alyapunov assigned locker and unassigned alyapunov Dec 12, 2022
@locker locker added the full-ci Enables all tests for a pull request label Dec 12, 2022
@locker
Copy link
Member Author

locker commented Dec 12, 2022

SHA1_Init is deprecated in OpenSSL 3.0. We should use EVP_DigestInit_ex instead.

@locker locker added do not merge Not ready to be merged and removed full-ci Enables all tests for a pull request labels Dec 12, 2022
Authentication salt is expected to be 20 byte long (SCRAMBLE_SIZE).
However, IPROTO sends 36 bytes (IPROTO_SALT_SIZE). Let's add a few
assertions ensuring that we never pass less than 20 bytes to
authentication methods.

Part of tarantool#7987

NO_DOC=code cleanup
NO_TEST=code cleanup
NO_CHANGELOG=code cleanup
Scramble is used only by the chap-sha1 authentication mechanism,
which has a proper API. There's no need in a standalone scramble lib.

Part of tarantool#7987

NO_DOC=code cleanup
NO_TEST=code cleanup
NO_CHANGELOG=code cleanup
We added an internal version of SHA1, because initially we didn't link
the OpenSSL library. However, since commit 59a5574 ("Link against
libssl and libcrypto. Issue tarantool#1382") we do so there's no need in it.

Part of tarantool#7987

NO_DOC=code cleanup
NO_TEST=code cleanup
NO_CHANGELOG=code cleanup
Since commit f6ea718 ("Try to load several variants of libssl.")
the digest module uses an internal version of SHA1. Back then, we didn't
link the OpenSSL library. Instead, we tried to load it dynamically.
Since on some distributions the library could be missing, it was decided
to implement an internal version of SHA1, see tarantool#405.

However, since commit 59a5574 ("Link against libssl and libcrypto.
Issue tarantool#1382") we link the OpenSSL library unconditionally so there's no
need in having an internal implementation of SHA1. Let's drop it and
switch the digest module to the version of SHA1 implemented by the
crypto module using OpenSSL.

Part of tarantool#7987

NO_DOC=code cleanup
NO_TEST=code cleanup
NO_CHANGELOG=code cleanup
Not used anywhere anymore. No need in it, because SHA1 is provided by
the OpenSSL library.

Closes tarantool#7987

NO_DOC=code cleanup
NO_TEST=code cleanup
NO_CHANGELOG=code cleanup
@locker
Copy link
Member Author

locker commented Dec 12, 2022

SHA1_Init is deprecated in OpenSSL 3.0. We should use EVP_DigestInit_ex instead.

Replaced with SHA1, which should be fine.

diff
diff --git a/src/box/auth_chap_sha1.c b/src/box/auth_chap_sha1.c
index 9ae4fee0aaa7..0665cf5dc2f5 100644
--- a/src/box/auth_chap_sha1.c
+++ b/src/box/auth_chap_sha1.c
@@ -84,17 +84,11 @@ scramble_prepare(void *out, const void *salt, const void *password,
 		 int password_len)
 {
 	unsigned char hash1[SCRAMBLE_SIZE];
-	unsigned char hash2[SCRAMBLE_SIZE];
-
 	SHA1(password, password_len, hash1);
-	SHA1(hash1, SCRAMBLE_SIZE, hash2);
-
-	SHA_CTX ctx;
-	SHA1_Init(&ctx);
-	SHA1_Update(&ctx, salt, SCRAMBLE_SIZE);
-	SHA1_Update(&ctx, hash2, SCRAMBLE_SIZE);
-	SHA1_Final(out, &ctx);
-
+	unsigned char salted_hash2[2 * SCRAMBLE_SIZE];
+	memcpy(salted_hash2, salt, SCRAMBLE_SIZE);
+	SHA1(hash1, SCRAMBLE_SIZE, salted_hash2 + SCRAMBLE_SIZE);
+	SHA1(salted_hash2, 2 * SCRAMBLE_SIZE, out);
 	xor(out, hash1, out, SCRAMBLE_SIZE);
 }
 
@@ -107,14 +101,11 @@ scramble_prepare(void *out, const void *salt, const void *password,
 static int
 scramble_check(const void *scramble, const void *salt, const void *hash2)
 {
+	unsigned char salted_hash2[2 * SCRAMBLE_SIZE];
+	memcpy(salted_hash2, salt, SCRAMBLE_SIZE);
+	memcpy(salted_hash2 + SCRAMBLE_SIZE, hash2, SCRAMBLE_SIZE);
 	unsigned char candidate_hash2[SCRAMBLE_SIZE];
-
-	SHA_CTX ctx;
-	SHA1_Init(&ctx);
-	SHA1_Update(&ctx, salt, SCRAMBLE_SIZE);
-	SHA1_Update(&ctx, hash2, SCRAMBLE_SIZE);
-	SHA1_Final(candidate_hash2, &ctx);
-
+	SHA1(salted_hash2, 2 * SCRAMBLE_SIZE, candidate_hash2);
 	xor(candidate_hash2, candidate_hash2, scramble, SCRAMBLE_SIZE);
 	/*
 	 * candidate_hash2 now supposedly contains hash1, turn it

@locker locker added full-ci Enables all tests for a pull request and removed do not merge Not ready to be merged labels Dec 12, 2022
@locker locker merged commit 3aafa0e into tarantool:master Dec 12, 2022
@locker locker deleted the drop-internal-sha1 branch December 12, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables all tests for a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace SHA1 implementation with OpenSSL's
3 participants