-
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
Conversation
@@ -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 |
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.
$SASL_DOMAIN = $SASL_REALM | ||
" > krb5.conf | ||
|
||
export KRB5_CONFIG=krb5.conf |
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 evergreen hosts come with an /etc/krb5.conf
file that is partially sufficient, but it does not include the cross-realm mapping and I wanted to test that since that is apparently not too uncommon a feature to be used by customers.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Since you can only kinit
one principal at a time, I figured this was the most straightforward way to skip the other cross-realm user principal test. Then, later in the file, we only run this test. Open to other ideas if you have a different opinion on how to handle this.
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() |
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.
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 false
, "forward"
, and "forwardAndReverse"
lookup for ldaptest.10gen.cc
all returned ldaptest.10gen.cc
. My initial rust implementation returned the "correct" results according the the spec algorithm/DNS definitions: ldaptest.10gen.cc
, ec2-54-225-237-121.compute-1.amazonaws.com.
, and ldaptest.10gen.cc.
, respectively. However, true FQDNs (with the trailing ".") are invalid in GSSAPI and most GSSAPI implementations do not support using canonical names that are different than the user-provided names (e.g. we can't replace ldaptest.10gen.cc
with the ec2 name and expect auth to work).
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.
Whoops put the wrong value in the secret vault for the name of the cross-realm db 😅 😓 updated and will re-run now. |
Do we know what's going on with these OIDC tests? Looks like some are also encountering system failures on the waterfall. |
That's a very good question. At a first look the failures seem unrelated both to your changes and each other; I've filed RUST-2252 for us to dig into them further. |
.evergreen/run-gssapi-tests.sh
Outdated
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 comment
The 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 /etc/hosts
on a shared test machine, won't this result in indefinitely re-appending the same hosts to the file ad nauseam?
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.
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 comment
The 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.
src/test/auth.rs
Outdated
@@ -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 comment
The 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 skip_local
convention that test modules like csfle do (https://github.com/mongodb/mongo-rust-driver/blob/main/src/test.rs#L20) so cargo nextest run
run locally will automatically skip the module:
mod gssapi; | |
#[path = "auth/gssapi.rs"] | |
mod gssapi_skip_local; |
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.
done
src/test/auth/gssapi.rs
Outdated
/// - 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 comment
The 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 comment
The 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 macro_rules!
is statically defined since it is expanded during the compilation process, not during runtime. As written, this file is 5 statically defined test functions. It's just that instead of using a single helper function they all effectively duplicate the same code over and over again.
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.
done
Sorry about the delay, I've been getting ahead of myself testing our Schema Manager tool and the ODBC driver with GSSAPI support. I made all the requested updates so far 👍 |
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.
LGTM!
This PR adds e2e testing for GSSAPI auth (covering
CANONICALIZE_HOST_NAME
options, as well as cross-realm auth) on linux and macOS (recall that GSSAPI auth has only been implemented on those two so far, Windows will be done in a follow-up ticket). I disabled the macOS tests for now given the current issues with getting mac hosts. I've tested locally on my mac, and everything has been working so I suspect the integration tests will work as well following the same pattern as the linux tests. Let me know if you'd prefer I enable them as part of this PR.I used the patterns described in the DBX wiki "Testing Kerberos". I checked other drivers and it seems there is inconsistent e2e GSSAPI testing. Some don't have any and many have limited testing. I covered cases I thought were most relevant, although this is clearly not an exhaustive test suite. Let me know if you want me to add anything else, although I'm not sure how much more is strictly necessary for our purposes at this time.