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

add TLS configuration server tests #254

Merged
merged 3 commits into from
Jan 22, 2018
Merged

add TLS configuration server tests #254

merged 3 commits into from
Jan 22, 2018

Conversation

aead
Copy link
Member

@aead aead commented Jan 15, 2018

This change adds non-functional tests to check whether a
minio endpoint (TLS) is configured properly.

This includes:

  • SSL/TLS version checks
  • Cipher suite checks

To separate security tests from functional tests this change adds a new subdirectory /run/security.

Fixes #253

Copy link
Contributor

@nitisht nitisht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition! added few comments, PTAL


test_run_dir="$MINT_RUN_TLS_DIR/go"
go get -u github.com/sirupsen/logrus/...
go build -o "$test_run_dir/server-tests" "$test_run_dir/server-tests.go"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline missing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

mint.sh Outdated
@@ -29,7 +29,7 @@ if [ -z "$SERVER_ENDPOINT" ]; then
fi

ROOT_DIR="$PWD"
TESTS_DIR="$ROOT_DIR/run/core"
TESTS_DIR="$ROOT_DIR/run"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing path issues for other tests here:https://github.com/minio/mint/blob/master/mint.sh#L116

You can add a new function run_security_test in this file, change run_test to run_core_test and call both run_core_test and run_security_test from main function, based on input parameters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the hint - will do that!

error_log_file="$2"

# run tests
/mint/run/tls/go/server-tests 1>>"$output_log_file" 2>"$error_log_file"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the path to /mint/run/tls/security/server-tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, changed. But path should be mint/run/security/tls/server-tests, right?!

Copy link
Contributor

@nitisht nitisht Jan 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, SECURITY_TESTS_DIR is set to "$ROOT_DIR/run/security", so either the directory should be named /mint/run/security or SECURITY_TESTS_DIR be set to "$ROOT_DIR/run/tls"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, TLS will be not the only security test. For example minio/minio#5411 was not TLS related.
So security tests under run/security - TLS is one class of security tests so run/security/tls.
Similar to multiple different SDKs under run/core.

mint.sh Outdated
fi
## Run security tests
echo -e "Running security tests:\n"
run_list=( "$SECURITY_TESTS_DIR"/* )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to update the count here as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

mint.sh Outdated
echo -e "\nExecuted $i out of $count tests successfully."
fi
## Run security tests
echo -e "Running security tests:\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add a check here to run this only if ENABLE_HTTPS is set

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added check. However if we add tests for recent V2 vulnerability we can run (some) security tests also against non-TLS endpoint. But for now it's fine I think.

mint.sh Outdated
@@ -29,7 +29,8 @@ if [ -z "$SERVER_ENDPOINT" ]; then
fi

ROOT_DIR="$PWD"
TESTS_DIR="$ROOT_DIR/run/core"
CORE_TESTS_DIR="$ROOT_DIR/run/core"
SECURITY_TESTS_DIR="$ROOT_DIR/run/security"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core under run directory is based on mint mode (I guess). Not sure why we need one more type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In run/core there are all functional tests - e.g. does a PUT work properly. In run/security there are non-functional security tests - e.g. is TLS configured properly. This has nothing to do with modes (standard vs. extensive, ...).

This change adds non-functional tests to check whether a
minio endpoint (TLS) is configured properly.

This includes:
 - SSL/TLS version checks
 - Cipher suite checks

To separate TLS tests from functional tests this change adds a new subdirectory `/run/tls`.

Fixes minio#253
@aead
Copy link
Member Author

aead commented Jan 22, 2018

Moved security tests to core as suggested by @nitisht. So the current proposal is security tests as non-functional tests (separate dir) as part of core. Different kinds of security tests (TLS, AdminAPI, ...) are implemented in different files - e.g. tls-tests.go, admin-api-tests.go, ...

@nitisht
Copy link
Contributor

nitisht commented Jan 22, 2018

Thanks @aead will test and approve

Copy link
Contributor

@nitisht nitisht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tested locally

@nitisht nitisht merged commit 787eb9a into minio:master Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants