End to end exhibitor lockdown #5985

jr0d · 2019-07-29T13:34:25Z

High-level description

Automated lock down of the exhibitor service via mutual TLS.

https://docs.mesosphere.com/1.13/security/ent/tls-ssl/exhibitor/

Adds the dcoscertstrap package which provides secure PKI bootstrap capabilities to the exhibitor service.
Orchestration for PKI initialization has been added to gen
start_exhibitor.py has been modified to generate end entity private keys and submit CSRs to the transient bootstrap CA service.

Corresponding DC/OS tickets (required)

DCOS-56036 Update installer for automated exhibitor lock down
DCOS-56038 Add Exhibitor lock down automation to exhibitor systemd unit

Related tickets (optional)

DCOS-55992 Develop install time CA CLI and service

Checklist for component/package updates:

If you are changing components or packages in DC/OS (e.g. you are bumping the sha or ref of anything underneath packages), then in addition to the above please also include:

Change log from the last version integrated (this should be a link to commits for easy verification and review): example
Included a test which will fail if code is reverted but test is not. If there is no test please explain.
Test Results: [link to CI job test results for component]
Code Coverage (if available): [link to code coverage report]

d2iq-mergebot · 2019-07-29T13:34:28Z

This repo has @mesosphere-mergebot integration. You can perform the following commands by submitting a comment. Submit a comment with content "@mesosphere-mergebot help" to view more detailed help text and examples. Be sure the have a look at the mergebot documentation, too.

@mesosphere-mergebot label [Request For Comment|Holding|Work In Progress|Ship It|merge-by-mergebot|Ready For Review] 
@mesosphere-mergebot changelog-not-required reason 
@mesosphere-mergebot sync  
@mesosphere-mergebot bump-ee  
@mesosphere-mergebot request-review  
@mesosphere-mergebot override-status pr-status-check jira-url 
@mesosphere-mergebot merge-it

PR creators can apply one of [Ready For Review|Work In Progress]. Owners can apply any label.

d2iq-mergebot · 2019-07-29T13:34:28Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-29T13:34:58Z

Enterprise Bump PR: mesosphere/dcos-enterprise/pull/6299

d2iq-mergebot · 2019-07-30T20:14:59Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-30T20:15:01Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

d2iq-mergebot · 2019-07-30T21:19:14Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-30T21:19:16Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

d2iq-mergebot · 2019-07-30T22:14:17Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-30T22:14:19Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

d2iq-mergebot · 2019-07-30T23:45:34Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-30T23:45:36Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

d2iq-mergebot · 2019-07-31T01:04:27Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-31T01:04:29Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

d2iq-mergebot · 2019-07-31T01:04:38Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-31T01:04:40Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

d2iq-mergebot · 2019-07-31T01:05:54Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-31T01:06:23Z

Enterprise Bump mesosphere/dcos-enterprise/pull/6299 updated.

d2iq-mergebot · 2019-07-31T12:08:00Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-31T12:08:26Z

Enterprise Bump mesosphere/dcos-enterprise/pull/6299 updated.

d2iq-mergebot · 2019-07-31T12:44:49Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-07-31T12:45:17Z

Enterprise Bump mesosphere/dcos-enterprise/pull/6299 updated.

d2iq-mergebot · 2019-08-01T23:23:45Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-08-01T23:23:47Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

d2iq-mergebot · 2019-08-02T02:14:58Z

@mesosphere-mergebot bump-ee

d2iq-mergebot · 2019-08-02T02:15:25Z

Enterprise Bump mesosphere/dcos-enterprise/pull/6299 updated.

d2iq-mergebot · 2019-08-02T18:19:18Z

@mesosphere-mergebot bump-ee

jgehrcke · 2019-08-15T09:20:51Z

gen/build_deploy/bash/dcos_generate_config.sh.in

+    echo
+fi
+
+exit 0


Before this patch we had exit $? (returning the exit status of docker run...?). Now we have exit 0. What's the rationale? Does not need to be answered before merging.

Since the script is running with set -e it only gets here if the exit code is 0

jgehrcke · 2019-08-15T09:22:02Z

gen/build_deploy/bash/dcos_generate_config.sh.in

@@ -50,4 +55,16 @@ else
    trap 'docker kill {genconf_tar}' HUP QUIT INT TERM
    docker run --rm --name=$DCOS_INSTALLER_CONTAINER_NAME -i -p $PORT:9000 -v $(pwd)/genconf/:/genconf {docker_image_name} "$@"
 fi
-exit $?
+
+if [ -d genconf/ca ]; then


Nit: Would be good to have a code comment here in which case genconf/ca is expected to be present, and in which case it's not expected to be present.

jgehrcke · 2019-08-15T09:22:51Z

gen/calc.py

+    if exhibitor_bootstrap_ca_url == '':
+        return
+
+    assert exhibitor_bootstrap_ca_url[-1] != '/', "Must not end in a '/'"


.endswith() :D

jgehrcke · 2019-08-15T09:24:25Z

gen/calc.py

@@ -1262,6 +1287,12 @@ def calculate_fault_domain_detect_contents(fault_domain_detect_filename):
        'superuser_service_account_uid': '',
        'superuser_service_account_public_key': '',
        '_superuser_service_account_public_key_json': calculate__superuser_service_account_public_key_json,
+        # This is a private variable that is not supposed to be used by cluster


"This" is easy to misunderstand. Instead of "This" let's write the name of the variable in the code comment. Can be done after merge.

jgehrcke · 2019-08-15T09:27:46Z

gen/exhibitor_tls_bootstrap.py

+         'CA init in gen is only supported when using a remote'
+         ' bootstrap node', False),
+        (lambda: 'exhibitor_bootstrap_ca_url' in config,
+         'Custom CA url has been specified, not initializing CA', False)


Lovely, thanks for putting error messages in a central place.

jgehrcke · 2019-08-15T09:28:53Z

gen/exhibitor_tls_bootstrap.py

+        # Exhibitor TLS is required, fail hard
+        if final_arguments.get('exhibitor_tls_required') == 'true' and hard_failure:
+            raise ExhibitorTLSBootstrapError(errors=reasons)
+        print('[{}] not bootstrapping exhibitor CA: {}'.format(


Nit: We should capitalize Exhibitor everywhere in docs and in error messages and in log messages, etc. Can do after merge.

jgehrcke · 2019-08-15T09:29:50Z

gen/exhibitor_tls_bootstrap.py

+        final_arguments['cluster_packages']) + '.tar.xz'
+    package_path = Path('/artifacts/packages') / PACKAGE_NAME / package_filename
+
+    ca_alternative_names = ['127.0.0.1', 'localhost',


Nit: should have a code comment for why each of the values is needed. Can do after merge.

jgehrcke · 2019-08-15T09:33:44Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+        ip = subprocess.check_output(
+            ['/opt/mesosphere/bin/detect_ip']).strip().decode('utf-8')
+    except subprocess.CalledProcessError as e:
+        print("check_output exited with {}".format(e))


Nit: will this really be a useful error message? Will it say something about the program that was executed? Can be tuned after merge.

I shamelessly copied this from somewhere else in the code base. I meant to reflector it :)

jgehrcke · 2019-08-15T09:34:07Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+    except socket.error as e:
+        print(
+            "inet_aton exited with {}. {} is not a valid IPv4 address".format(e, ip))
+        sys.exit(1)


Is that validation that we need here? Isn't that done elsewhere?

jgehrcke · 2019-08-15T09:34:34Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+
+def test_connection(ca_url):
+    s = socket.socket()
+    s.settimeout(5)


is this the connect() or the recv() timeout, or both?

I mean, does this really set the connect() timeout, which is what we seem to care about here to not hang forever...

We're good:

The connect() operation is also subject to the timeout setting, and in general it is recommended to call settimeout() before calling connect()

https://docs.python.org/3/library/socket.html#timeouts-and-the-connect-method

jgehrcke · 2019-08-15T09:36:49Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+        print('could not connect to DC/OS bootstrap CA: {}'.format(e))
+        return False
+    finally:
+        s.close()


jgehrcke · 2019-08-15T09:37:04Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+        print('connection to {}:{} successful'.format(host, port))
+        return True
+    except Exception as e:
+        print('could not connect to DC/OS bootstrap CA: {}'.format(e))


decent error handling, thanks!

jgehrcke · 2019-08-15T09:38:44Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+    if not test_connection(ca_url):
+        _fail_and_calculate_status('connection failed')
+
+    print('Bootstrapping exhibitor TLS')


Again: let's capitalize.... (can do after merge)

jgehrcke · 2019-08-15T09:39:24Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+        # Clean up any partially written artifacts
+        _remove_artifact_dir()
+        _fail_and_calculate_status(
+            'error generating artifacts code: {} stdout: {} stderr: {}'.format(


After merge, let's polish:

Error generating Exhibitor TLS artifacts. Exit code: ...

jgehrcke · 2019-08-15T09:40:24Z

packages/exhibitor/extra/bootstrap_exhibitor_tls.py

+if __name__ == '__main__':
+    main()
+    # Always flush stdout buffer when exiting the script
+    sys.stdout.flush()


mhm if this has been a problem I recommend starting the Python interperter with the environment variable PYTHONIOBUFFERING set so that std stream buffering gets disabled completely.

jgehrcke

Lovely work, nice code base. Let's get this merged and iterate on it thereafter.

jongiddy

Thanks everyone. Let's merge and fix the nits later.

jgehrcke · 2019-08-15T10:25:08Z

@mesosphere-mergebot changelog-not-required This is covered in the downstream changelog

jgehrcke · 2019-08-15T10:26:28Z

Applying the ship-it label manually and merging right away, this is highly unlikely to be in logical conflict with the changes to master from the last two days.

jgehrcke · 2019-08-15T10:26:39Z

@mesosphere-mergebot label ship-it

jgehrcke · 2019-08-15T10:26:52Z

@mesosphere-mergebot merge-it

d2iq-mergebot · 2019-08-15T10:26:54Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

jgehrcke · 2019-08-15T10:27:37Z

Your pull request's branch is not based on the most recent version of master. Please rebase your changes against this repo's master branch.

I thought merge-it allows humans to be smarter than machines. Okay, I am not pulling off the completely manual merge now, so let's wait for a train to kick in.

d2iq-mergebot added the Work In Progress label Jul 29, 2019

jr0d force-pushed the jr/exhibitor-lockdown-automation-future branch from 142ac50 to c517762 Compare July 31, 2019 01:05

jr0d force-pushed the jr/exhibitor-lockdown-automation-future branch from c517762 to 24a9141 Compare July 31, 2019 12:07

jr0d force-pushed the jr/exhibitor-lockdown-automation-future branch from 05577b2 to c671585 Compare August 2, 2019 02:14

jgehrcke reviewed Aug 15, 2019

View reviewed changes

jgehrcke approved these changes Aug 15, 2019

View reviewed changes

jongiddy approved these changes Aug 15, 2019

View reviewed changes

d2iq-mergebot added Ship It and removed Ready For Review labels Aug 15, 2019

d2iq-mergebot added a commit to mesosphere/dcos that referenced this pull request Aug 15, 2019

Merge dcos#5985

453dbd9

d2iq-mergebot added a commit to mesosphere/dcos that referenced this pull request Aug 15, 2019

Merge dcos#5985

841e4aa

d2iq-mergebot added a commit to mesosphere/dcos that referenced this pull request Aug 15, 2019

Merge dcos#5985

e0d8908

d2iq-mergebot mentioned this pull request Aug 15, 2019

[master] Mergebot Automated Train PR - 2019-Aug-15-10-33 #6083

Merged

d2iq-mergebot merged commit 87aeb7e into dcos:master Aug 15, 2019

+                  echo
+              fi
+              exit 0

End to end exhibitor lockdown #5985

End to end exhibitor lockdown #5985

Conversation

jr0d commented Jul 29, 2019

High-level description

Corresponding DC/OS tickets (required)

Related tickets (optional)

Checklist for component/package updates:

d2iq-mergebot commented Jul 29, 2019

d2iq-mergebot commented Jul 29, 2019

d2iq-mergebot commented Jul 29, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 30, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Jul 31, 2019

d2iq-mergebot commented Aug 1, 2019

d2iq-mergebot commented Aug 1, 2019

d2iq-mergebot commented Aug 2, 2019

d2iq-mergebot commented Aug 2, 2019

d2iq-mergebot commented Aug 2, 2019

jgehrcke Aug 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgehrcke Aug 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgehrcke Aug 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgehrcke left a comment

Choose a reason for hiding this comment

jongiddy left a comment

Choose a reason for hiding this comment

jgehrcke commented Aug 15, 2019

jgehrcke commented Aug 15, 2019

jgehrcke commented Aug 15, 2019

jgehrcke commented Aug 15, 2019

d2iq-mergebot commented Aug 15, 2019

jgehrcke commented Aug 15, 2019

jgehrcke Aug 15, 2019 •

edited

jgehrcke Aug 15, 2019 •

edited

jgehrcke Aug 15, 2019 •

edited