-
Notifications
You must be signed in to change notification settings - Fork 5.4k
OAuth2: PKCE(Proof Key for Code Exchange) #37849
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
Changes from 11 commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
21313ef
api for pkce(Proof Key for Code Exchange)
zhaohuabing 7dbbde5
implementation
zhaohuabing c008e80
update tests
zhaohuabing aac2652
minor wording
zhaohuabing 0dd6e3a
add change log
zhaohuabing 60e5bab
fix format
zhaohuabing 6df85ea
fix test
zhaohuabing 62f5537
remove enable_pkce
zhaohuabing 73860ef
Merge branch 'main' into api-ouath-pkce
zhaohuabing 3cf2b43
encrypt the code verifier
zhaohuabing 009cbee
Merge remote-tracking branch 'origin/main' into api-ouath-pkce
zhaohuabing bfdd457
fix integration test
zhaohuabing 06ee3f1
address comment
zhaohuabing e45cf89
minor change
zhaohuabing d0402b5
Merge remote-tracking branch 'origin/main' into api-ouath-pkce
zhaohuabing 2eb97c5
fix test
zhaohuabing fa02bb3
fix test
zhaohuabing db8f193
Merge remote-tracking branch 'origin/main' into api-ouath-pkce
zhaohuabing daa9271
change log
zhaohuabing 765b479
change log
zhaohuabing 248af96
add test
zhaohuabing 31da834
fix format
zhaohuabing 67e1237
fix test
zhaohuabing 47e9be9
fix test
zhaohuabing dae8f2b
add more tests
zhaohuabing efa59da
minor change
zhaohuabing 7eeffc1
fix verify
zhaohuabing 3e447b4
update patch
zhaohuabing 2d4baf3
call EVP_CIPHER_CTX_free(ctx) when decryption fails
zhaohuabing ac3b502
Merge branch 'main' into api-ouath-pkce
zhaohuabing 05a54cd
Merge remote-tracking branch 'origin/main' into api-ouath-pkce
zhaohuabing 69211f5
Revert "fix verify"
zhaohuabing File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| #include "absl/strings/str_split.h" | ||
| #include "jwt_verify_lib/jwt.h" | ||
| #include "jwt_verify_lib/status.h" | ||
| #include "openssl/rand.h" | ||
|
|
||
| using namespace std::chrono_literals; | ||
|
|
||
|
|
@@ -50,6 +51,8 @@ constexpr absl::string_view queryParamsError = "error"; | |
| constexpr absl::string_view queryParamsCode = "code"; | ||
| constexpr absl::string_view queryParamsState = "state"; | ||
| constexpr absl::string_view queryParamsRedirectUri = "redirect_uri"; | ||
| constexpr absl::string_view queryParamsCodeChallenge = "code_challenge"; | ||
| constexpr absl::string_view queryParamsCodeChallengeMethod = "code_challenge_method"; | ||
|
|
||
| constexpr absl::string_view stateParamsUrl = "url"; | ||
| constexpr absl::string_view stateParamsCsrfToken = "csrf_token"; | ||
|
|
@@ -203,6 +206,30 @@ bool validateCsrfTokenHmac(const std::string& hmac_secret, const std::string& cs | |
| return generateHmacBase64(hmac_secret_vec, token) == hmac; | ||
| } | ||
|
|
||
| // Generates a PKCE code verifier with 32 octets of randomness. | ||
| // This follows recommendations in RFC 7636: | ||
| // https://datatracker.ietf.org/doc/html/rfc7636#section-7.1 | ||
| std::string generateCodeVerifier(Random::RandomGenerator& random) { | ||
| MemBlockBuilder<uint64_t> mem_block(4); | ||
| // create 4 random uint64_t values to fill the buffer because RFC 7636 recommends 32 octets of | ||
| // randomness. | ||
| for (size_t i = 0; i < 4; i++) { | ||
| mem_block.appendOne(random.random()); | ||
| } | ||
|
|
||
| std::unique_ptr<uint64_t[]> data = mem_block.release(); | ||
| return Base64Url::encode(reinterpret_cast<char*>(data.get()), 4 * sizeof(uint64_t)); | ||
| } | ||
|
|
||
| // Generates a PKCE code challenge from a code verifier. | ||
| std::string generateCodeChallenge(const std::string& code_verifier) { | ||
| auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get(); | ||
| std::vector<uint8_t> sha256_digest = | ||
| crypto_util.getSha256Digest(Buffer::OwnedImpl(code_verifier)); | ||
| std::string sha256_string(sha256_digest.begin(), sha256_digest.end()); | ||
| return Base64Url::encode(sha256_string.data(), sha256_string.size()); | ||
| } | ||
|
|
||
| /** | ||
| * Encodes the state parameter for the OAuth2 flow. | ||
| * The state parameter is a base64Url encoded JSON object containing the original request URL and a | ||
|
|
@@ -217,6 +244,128 @@ std::string encodeState(const std::string& original_request_url, const std::stri | |
| return Base64Url::encode(json.data(), json.size()); | ||
| } | ||
|
|
||
| /** | ||
| * Encrypt a plaintext string using AES-256-CBC. | ||
| */ | ||
| std::string encrypt(const std::string& plaintext, const std::string& secret, | ||
| Random::RandomGenerator& random) { | ||
| // Generate the key from the secret using SHA-256 | ||
| std::vector<unsigned char> key(SHA256_DIGEST_LENGTH); // AES-256 requires 256-bit (32 bytes) key | ||
| SHA256(reinterpret_cast<const unsigned char*>(secret.c_str()), secret.size(), key.data()); | ||
|
|
||
| // Generate a random IV | ||
| MemBlockBuilder<uint64_t> mem_block(4); | ||
| // create 2 random uint64_t values to fill the buffer because AES-256-CBC requires 16 bytes IV | ||
| for (size_t i = 0; i < 2; i++) { | ||
| mem_block.appendOne(random.random()); | ||
| } | ||
|
|
||
| std::unique_ptr<uint64_t[]> data = mem_block.release(); | ||
| const unsigned char* raw_data = reinterpret_cast<const unsigned char*>(data.get()); | ||
|
|
||
| std::vector<unsigned char> iv(16); // AES uses 16-byte IV | ||
| iv.assign(raw_data, raw_data + 16); | ||
|
|
||
| EVP_CIPHER_CTX* ctx = EVP_CIPHER_CTX_new(); | ||
| if (!ctx) { | ||
| throw EnvoyException("Failed to create context"); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should never use exception execept loading configuration. Maybe |
||
|
|
||
| std::vector<unsigned char> ciphertext(plaintext.size() + EVP_MAX_BLOCK_LENGTH); | ||
| int len = 0, ciphertext_len = 0; | ||
|
|
||
| // Initialize encryption operation | ||
| if (EVP_EncryptInit_ex(ctx, EVP_aes_256_cbc(), nullptr, key.data(), iv.data()) != 1) { | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| throw EnvoyException("Encryption initialization failed"); | ||
| } | ||
|
|
||
| // Encrypt the plaintext | ||
| if (EVP_EncryptUpdate(ctx, ciphertext.data(), &len, | ||
| reinterpret_cast<const unsigned char*>(plaintext.c_str()), | ||
| plaintext.size()) != 1) { | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| throw EnvoyException("Encryption update failed"); | ||
| } | ||
| ciphertext_len += len; | ||
|
|
||
| // Finalize encryption | ||
| if (EVP_EncryptFinal_ex(ctx, ciphertext.data() + len, &len) != 1) { | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| throw EnvoyException("Encryption finalization failed"); | ||
| } | ||
| ciphertext_len += len; | ||
|
|
||
| EVP_CIPHER_CTX_free(ctx); | ||
|
|
||
| ciphertext.resize(ciphertext_len); // Resize to actual length | ||
|
|
||
| // Prepend the IV to the ciphertext | ||
| std::vector<unsigned char> combined(iv.size() + ciphertext.size()); | ||
| std::copy(iv.begin(), iv.end(), combined.begin()); | ||
| std::copy(ciphertext.begin(), ciphertext.end(), combined.begin() + iv.size()); | ||
|
|
||
| // Base64Url encode the IV + ciphertext | ||
| return Base64Url::encode(reinterpret_cast<const char*>(combined.data()), combined.size()); | ||
| } | ||
|
|
||
| /** | ||
| * Decrypt an AES-256-CBC encrypted string. | ||
| */ | ||
| std::string decrypt(const std::string& encrypted, const std::string& secret) { | ||
| // Decode the Base64Url-encoded input | ||
| std::string decoded = Base64Url::decode(encrypted); | ||
| std::vector<unsigned char> combined(decoded.begin(), decoded.end()); | ||
|
|
||
| if (combined.size() <= 16) { | ||
| throw EnvoyException("Invalid encrypted data"); | ||
| } | ||
|
|
||
| // Extract the IV (first 16 bytes) | ||
| std::vector<unsigned char> iv(combined.begin(), combined.begin() + 16); | ||
|
|
||
| // Extract the ciphertext (remaining bytes) | ||
| std::vector<unsigned char> ciphertext(combined.begin() + 16, combined.end()); | ||
|
|
||
| // Generate the key from the secret using SHA-256 | ||
| std::vector<unsigned char> key(SHA256_DIGEST_LENGTH); // 256-bit key | ||
| SHA256(reinterpret_cast<const unsigned char*>(secret.c_str()), secret.size(), key.data()); | ||
|
|
||
| EVP_CIPHER_CTX* ctx = EVP_CIPHER_CTX_new(); | ||
| if (!ctx) { | ||
| throw EnvoyException("Failed to create context"); | ||
| } | ||
|
|
||
| std::vector<unsigned char> plaintext(ciphertext.size() + EVP_MAX_BLOCK_LENGTH); | ||
| int len = 0, plaintext_len = 0; | ||
|
|
||
| // Initialize decryption operation | ||
| if (EVP_DecryptInit_ex(ctx, EVP_aes_256_cbc(), nullptr, key.data(), iv.data()) != 1) { | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| throw EnvoyException("Decryption initialization failed"); | ||
| } | ||
|
|
||
| // Decrypt the ciphertext | ||
| if (EVP_DecryptUpdate(ctx, plaintext.data(), &len, ciphertext.data(), ciphertext.size()) != 1) { | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| throw EnvoyException("Decryption update failed"); | ||
| } | ||
| plaintext_len += len; | ||
|
|
||
| // Finalize decryption | ||
| if (EVP_DecryptFinal_ex(ctx, plaintext.data() + len, &len) != 1) { | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| throw EnvoyException("Decryption finalization failed"); | ||
| } | ||
| plaintext_len += len; | ||
|
|
||
| EVP_CIPHER_CTX_free(ctx); | ||
|
|
||
| plaintext.resize(plaintext_len); // Resize to actual plaintext length | ||
|
|
||
| return std::string(plaintext.begin(), plaintext.end()); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| FilterConfig::FilterConfig( | ||
|
|
@@ -464,8 +613,18 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he | |
| Formatter::FormatterImpl::create(config_->redirectUri()), Formatter::FormatterPtr); | ||
| const auto redirect_uri = | ||
| formatter->formatWithContext({&headers}, decoder_callbacks_->streamInfo()); | ||
|
|
||
| std::string encrypted_code_verifier = | ||
| Http::Utility::parseCookieValue(headers, config_->cookieNames().code_verifier_); | ||
| if (encrypted_code_verifier.empty()) { | ||
| ENVOY_LOG(error, "code verifier cookie is missing in the request"); | ||
| sendUnauthorizedResponse(); | ||
| return Http::FilterHeadersStatus::StopIteration; | ||
| } | ||
| std::string code_verifier = decrypt(encrypted_code_verifier, config_->hmacSecret()); | ||
|
|
||
| oauth_client_->asyncGetAccessToken(auth_code_, config_->clientId(), config_->clientSecret(), | ||
| redirect_uri, config_->authType()); | ||
| redirect_uri, code_verifier, config_->authType()); | ||
|
|
||
| // pause while we await the next step from the OAuth server | ||
| return Http::FilterHeadersStatus::StopAllIterationAndBuffer; | ||
|
|
@@ -526,22 +685,13 @@ void OAuth2Filter::redirectToOAuthServer(Http::RequestHeaderMap& headers) { | |
| // The CSRF token cookie contains the CSRF token that is used to prevent CSRF attacks for the | ||
| // OAuth flow. It was named "oauth_nonce" because the CSRF token contains a generated nonce. | ||
| // "oauth_csrf_token" would be a more accurate name for the cookie. | ||
| std::string csrf_token; | ||
| bool csrf_token_cookie_exists = false; | ||
| const auto csrf_token_cookie = | ||
| Http::Utility::parseCookies(headers, [this](absl::string_view key) { | ||
| return key == config_->cookieNames().oauth_nonce_; | ||
| }); | ||
| if (csrf_token_cookie.find(config_->cookieNames().oauth_nonce_) != csrf_token_cookie.end()) { | ||
| csrf_token = csrf_token_cookie.at(config_->cookieNames().oauth_nonce_); | ||
| csrf_token_cookie_exists = true; | ||
| } else { | ||
| // Generate a CSRF token to prevent CSRF attacks. | ||
| csrf_token = generateCsrfToken(config_->hmacSecret(), random_); | ||
| } | ||
|
|
||
| std::string csrf_token = | ||
| Http::Utility::parseCookieValue(headers, config_->cookieNames().oauth_nonce_); | ||
| bool csrf_token_cookie_exists = !csrf_token.empty(); | ||
| // Set the CSRF token cookie if it does not exist. | ||
| if (!csrf_token_cookie_exists) { | ||
| // Generate a CSRF token to prevent CSRF attacks. | ||
| csrf_token = generateCsrfToken(config_->hmacSecret(), random_); | ||
| // Expire the CSRF token cookie in 10 minutes. | ||
| // This should be enough time for the user to complete the OAuth flow. | ||
| std::string expire_in = std::to_string(10 * 60); | ||
|
|
@@ -574,6 +724,28 @@ void OAuth2Filter::redirectToOAuthServer(Http::RequestHeaderMap& headers) { | |
| Http::Utility::PercentEncoding::urlEncodeQueryParameter(redirect_uri); | ||
| query_params.overwrite(queryParamsRedirectUri, escaped_redirect_uri); | ||
|
|
||
| // Generate a PKCE code verifier and challenge for the OAuth flow. | ||
| const std::string code_verifier = generateCodeVerifier(random_); | ||
| // Encrypt the code verifier, using HMAC secret as the symmetric key. | ||
| const std::string encrypted_code_verifier = | ||
| encrypt(code_verifier, config_->hmacSecret(), random_); | ||
|
|
||
| // Expire the code verifier cookie in 10 minutes. | ||
| // This should be enough time for the user to complete the OAuth flow. | ||
| std::string expire_in = std::to_string(10 * 60); | ||
| std::string cookie_tail_http_only = fmt::format(CookieTailHttpOnlyFormatString, expire_in); | ||
| if (!config_->cookieDomain().empty()) { | ||
| cookie_tail_http_only = absl::StrCat( | ||
| fmt::format(CookieDomainFormatString, config_->cookieDomain()), cookie_tail_http_only); | ||
| } | ||
| response_headers->addReferenceKey(Http::Headers::get().SetCookie, | ||
| absl::StrCat(config_->cookieNames().code_verifier_, "=", | ||
| encrypted_code_verifier, cookie_tail_http_only)); | ||
|
|
||
| const std::string code_challenge = generateCodeChallenge(code_verifier); | ||
| query_params.overwrite(queryParamsCodeChallenge, code_challenge); | ||
| query_params.overwrite(queryParamsCodeChallengeMethod, "S256"); | ||
|
|
||
|
zhaohuabing marked this conversation as resolved.
|
||
| // Copy the authorization endpoint URL to replace its query params. | ||
| auto authorization_endpoint_url = config_->authorizationEndpointUrl(); | ||
| const std::string path_and_query_params = query_params.replaceQueryString( | ||
|
|
@@ -621,6 +793,10 @@ Http::FilterHeadersStatus OAuth2Filter::signOutUser(const Http::RequestHeaderMap | |
| Http::Headers::get().SetCookie, | ||
| absl::StrCat(fmt::format(CookieDeleteFormatString, config_->cookieNames().oauth_nonce_), | ||
| cookie_domain)); | ||
| response_headers->addReferenceKey( | ||
| Http::Headers::get().SetCookie, | ||
| absl::StrCat(fmt::format(CookieDeleteFormatString, config_->cookieNames().code_verifier_), | ||
| cookie_domain)); | ||
| response_headers->setLocation(new_path); | ||
| decoder_callbacks_->encodeHeaders(std::move(response_headers), true, SIGN_OUT); | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The encrpt and decrpt method can be also applied to the ID and Access tokens to provide an addtional layer of protection. We can address this in a follow-up PR.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there already any active development efforts related to this?