-
Notifications
You must be signed in to change notification settings - Fork 181
RUST-2236 Add e2e testing for GSSAPI auth on Linux and macOS #1431
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
Changes from 9 commits
b80350a
b4812ff
bd5d723
84d1eba
c9f3bb1
f072e6f
f8b0f09
96e1deb
4043959
c962218
75c8479
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,62 @@ cd ${PROJECT_DIRECTORY} | |
source .evergreen/env.sh | ||
source .evergreen/cargo-test.sh | ||
|
||
# Source the drivers/atlas_connect secrets, where GSSAPI test values are held | ||
source "${DRIVERS_TOOLS}/.evergreen/secrets_handling/setup-secrets.sh" drivers/atlas_connect | ||
|
||
FEATURE_FLAGS+=("gssapi-auth") | ||
|
||
set +o errexit | ||
|
||
# Update hosts with relevant data | ||
echo "`host $SASL_HOST | awk '/has address/ { print $4 ; exit }'` $SASL_HOST" | sudo tee -a /etc/hosts | ||
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. I'm a bit worried about this line - aside from the impolite nature of updating 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. You know, this might be a gap in my evergreen knowledge: I was under the impression hosts were wiped clean between runs. I can update this to only do it if the hosts file doesn't already contain the relevant mapping 👍 that's the safest option. This command is based on the drivers wiki page I referenced in the PR summary -- I suspect they had local testing in mind -- not evergreen testing -- when they wrote that. 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. Actually, it seems that by default all hosts will have this mapping (at least the hosts we're using for the test) since it is a common test server owned by dev prod and used by many teams. Given that, I just removed this line entirely. I'm assuming the tests will still pass without this line -- we'll see on this run. If they do not because of this, I'll go with the plan I outlined in my previous comment. |
||
|
||
# Create a krb5 config file with relevant | ||
touch krb5.conf | ||
echo "[realms] | ||
$SASL_REALM = { | ||
kdc = $SASL_HOST | ||
admin_server = $SASL_HOST | ||
} | ||
|
||
$SASL_REALM_CROSS = { | ||
kdc = $SASL_HOST | ||
admin_server = $SASL_HOST | ||
} | ||
|
||
[domain_realm] | ||
.$SASL_DOMAIN = $SASL_REALM | ||
$SASL_DOMAIN = $SASL_REALM | ||
" > krb5.conf | ||
|
||
export KRB5_CONFIG=krb5.conf | ||
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. The evergreen hosts come with an |
||
|
||
# Authenticate the user principal in the KDC before running the e2e test | ||
echo "Authenticating $PRINCIPAL" | ||
echo "$SASL_PASS" | kinit -p $PRINCIPAL | ||
klist | ||
|
||
# Run end-to-end auth tests for "$PRINCIPAL" user | ||
TEST_OPTIONS+=("--skip with_service_realm_and_host_options") | ||
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. Since you can only |
||
cargo_test test::auth::gssapi | ||
|
||
# Unauthenticate | ||
echo "Unauthenticating $PRINCIPAL" | ||
kdestroy | ||
|
||
# Authenticate the alternative user principal in the KDC and run other e2e test | ||
echo "Authenticating $PRINCIPAL_CROSS" | ||
echo "$SASL_PASS_CROSS" | kinit -p $PRINCIPAL_CROSS | ||
klist | ||
|
||
TEST_OPTIONS=() | ||
cargo_test test::auth::gssapi::with_service_realm_and_host_options | ||
|
||
# Unauthenticate | ||
echo "Unuthenticating $PRINCIPAL_CROSS" | ||
kdestroy | ||
|
||
# Run remaining tests | ||
cargo_test spec::auth | ||
cargo_test uri_options | ||
cargo_test connection_string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
use cross_krb5::{ClientCtx, InitiateFlags, K5Ctx, PendingClientCtx, Step}; | ||
use hickory_resolver::proto::rr::RData; | ||
|
||
use crate::{ | ||
bson::Bson, | ||
|
@@ -324,21 +323,24 @@ async fn canonicalize_hostname( | |
let resolver = | ||
crate::runtime::AsyncResolver::new(resolver_config.map(|c| c.inner.clone())).await?; | ||
|
||
match mode { | ||
let hostname = match mode { | ||
CanonicalizeHostName::Forward => { | ||
let lookup_records = resolver.cname_lookup(hostname).await?; | ||
|
||
if let Some(first_record) = lookup_records.records().first() { | ||
if let Some(RData::CNAME(cname)) = first_record.data() { | ||
Ok(cname.to_lowercase().to_string()) | ||
} else { | ||
Ok(hostname.to_string()) | ||
} | ||
if !lookup_records.records().is_empty() { | ||
// As long as there is a record, we can return the original hostname. | ||
// Although the spec says to return the canonical name, this is not | ||
// done by any drivers in practice since the majority of them use | ||
// libraries that do not follow CNAME chains. Also, we do not want to | ||
// use the canonical name since it will likely differ from the input | ||
// name, and the use of the input name is required for the service | ||
// principal to be accepted by the GSSAPI auth flow. | ||
hostname.to_lowercase().to_string() | ||
Comment on lines
+330
to
+338
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. This was certainly the most interesting find of this ticket. Initially, I did properly implement the spec algorithm for getting canonical names for hosts, however it turns out no drivers actually use that algorithm since GSSAPI often does not support the use of canonical names. I discussed this with mongoGPT and thought it may be hallucinating but I then checked the other drivers and it appears to be correct. Using the python driver as a source of truth, for example, I saw that Given all those findings, I updated this function to be in-line with the python implementation more directly. It now matches the results the python implementation produces. |
||
} else { | ||
Err(Error::authentication_error( | ||
return Err(Error::authentication_error( | ||
GSSAPI_STR, | ||
&format!("No addresses found for hostname: {hostname}"), | ||
)) | ||
)); | ||
} | ||
} | ||
CanonicalizeHostName::ForwardAndReverse => { | ||
|
@@ -350,20 +352,27 @@ async fn canonicalize_hostname( | |
match resolver.reverse_lookup(first_address).await { | ||
Ok(reverse_lookup) => { | ||
if let Some(name) = reverse_lookup.iter().next() { | ||
Ok(name.to_lowercase().to_string()) | ||
name.to_lowercase().to_string() | ||
} else { | ||
Ok(hostname.to_lowercase()) | ||
hostname.to_lowercase() | ||
} | ||
} | ||
Err(_) => Ok(hostname.to_lowercase()), | ||
Err(_) => hostname.to_lowercase(), | ||
} | ||
} else { | ||
Err(Error::authentication_error( | ||
return Err(Error::authentication_error( | ||
GSSAPI_STR, | ||
&format!("No addresses found for hostname: {hostname}"), | ||
)) | ||
)); | ||
} | ||
} | ||
CanonicalizeHostName::None => unreachable!(), | ||
} | ||
}; | ||
|
||
// Sometimes reverse lookup results in a trailing "." since that is the correct | ||
// way to present a FQDN. However, GSSAPI rejects the trailing "." so we remove | ||
// it here manually. | ||
let hostname = hostname.trim_end_matches("."); | ||
|
||
Ok(hostname.to_string()) | ||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,7 @@ | ||||||||
#[cfg(feature = "aws-auth")] | ||||||||
mod aws; | ||||||||
#[cfg(feature = "gssapi-auth")] | ||||||||
mod gssapi; | ||||||||
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. Since the tests in here require external setup, I think this should use the same
Suggested change
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. done |
||||||||
|
||||||||
use serde::Deserialize; | ||||||||
|
||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
use crate::{ | ||
bson::{doc, Document}, | ||
Client, | ||
}; | ||
|
||
/// Create a GSSAPI e2e test. | ||
/// - test_name is the name of the test. | ||
/// - user_principal is the name of the environment variable that stores the user principal | ||
/// - auth_mechanism_properties is an optional set of authMechanismProperties to append to the uri | ||
macro_rules! test_gssapi_auth { | ||
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. Does this need to be a macro? It looks like aside from test name the parameters here could be normal function parameters to a helper function called by statically defined test functions. 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. This was just force of habit since this is the standard way we define tests in all our Rust repos on SQL Engines. I'll update this to be more in-line with the conventions of this repo. As an aside, code generated by 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. done |
||
($test_name:ident, user_principal = $user_principal:expr, gssapi_db = $gssapi_db:expr, $(auth_mechanism_properties = $auth_mechanism_properties:expr,)?) => { | ||
#[tokio::test] | ||
async fn $test_name() { | ||
// Get env variables | ||
let host = std::env::var("SASL_HOST").expect("SASL_HOST not set"); | ||
let user_principal = std::env::var($user_principal) | ||
.expect(format!("{} not set", $user_principal).as_str()) | ||
.replace("@", "%40"); | ||
let gssapi_db = std::env::var($gssapi_db) | ||
.expect(format!("{} not set", $gssapi_db).as_str()); | ||
|
||
// Optionally create authMechanismProperties | ||
#[allow(unused_mut, unused_assignments)] | ||
let mut auth_mechanism_properties = "".to_string(); | ||
$(auth_mechanism_properties = format!("&authMechanismProperties={}", $auth_mechanism_properties);)? | ||
|
||
// Create client | ||
let uri = format!("mongodb://{user_principal}@{host}/?authSource=%24external&authMechanism=GSSAPI{auth_mechanism_properties}"); | ||
let client = Client::with_uri_str(uri) | ||
.await | ||
.expect("failed to create MongoDB Client"); | ||
|
||
// Check that auth worked by qurying the test collection | ||
let coll = client.database(&gssapi_db).collection::<Document>("test"); | ||
let doc = coll.find_one(doc! {}).await; | ||
match doc { | ||
Ok(Some(doc)) => { | ||
assert!( | ||
doc.get_bool(&gssapi_db).unwrap(), | ||
"expected '{gssapi_db}' field to exist and be 'true'" | ||
); | ||
assert_eq!( | ||
doc.get_str("authenticated").unwrap(), | ||
"yeah", | ||
"unexpected 'authenticated' value" | ||
); | ||
} | ||
Ok(None) => panic!("expected `find_one` to return a document, but it did not"), | ||
Err(e) => panic!("expected `find_one` to return a document, but it failed: {e:?}"), | ||
} | ||
} | ||
}; | ||
} | ||
|
||
test_gssapi_auth!( | ||
no_options, | ||
user_principal = "PRINCIPAL", | ||
gssapi_db = "GSSAPI_DB", | ||
); | ||
|
||
test_gssapi_auth!( | ||
explicit_canonicalize_host_name_false, | ||
user_principal = "PRINCIPAL", | ||
gssapi_db = "GSSAPI_DB", | ||
auth_mechanism_properties = "CANONICALIZE_HOST_NAME:false", | ||
); | ||
|
||
test_gssapi_auth!( | ||
canonicalize_host_name_forward, | ||
user_principal = "PRINCIPAL", | ||
gssapi_db = "GSSAPI_DB", | ||
auth_mechanism_properties = "CANONICALIZE_HOST_NAME:forward", | ||
); | ||
|
||
test_gssapi_auth!( | ||
canonicalize_host_name_forward_and_reverse, | ||
user_principal = "PRINCIPAL", | ||
gssapi_db = "GSSAPI_DB", | ||
auth_mechanism_properties = "CANONICALIZE_HOST_NAME:forwardAndReverse", | ||
); | ||
|
||
test_gssapi_auth!( | ||
with_service_realm_and_host_options, | ||
// Use the cross-realm USER principal | ||
user_principal = "PRINCIPAL_CROSS", | ||
// The cross-realm user is only authorized on the cross-realm db | ||
gssapi_db = "GSSAPI_DB_CROSS", | ||
auth_mechanism_properties = { | ||
// However, the SERVICE principal is not cross-realm, hence the use of | ||
// SASL_REALM instead of SASL_REALM_CROSS. | ||
let service_realm = std::env::var("SASL_REALM").expect("SASL_REALM not set"); | ||
let service_host = std::env::var("SASL_HOST").expect("SASL_HOST not set"); | ||
format!("SERVICE_REALM:{service_realm},SERVICE_HOST:{service_host}").as_str() | ||
}, | ||
); |
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.
drivers/atlas_connect
already had half of the secrets I needed, so I chose that vault and also added the remaining secrets I needed.