Feat/ota 2666/garage tools mutual tls #1243

pattivacek · 2019-06-25T15:47:50Z

Short version: it works! Well, it works as far as the unit tests are concerned. The certs were generated by the Makefile that @simao provided and @koshelev, @jochenschneider, and @zabbal consulted with.

Long version: This still hasn't been tested with any real server implementation. Also, this PR currently violates my preference to keep binaries out of git. I was able to add some scripts to generate the zip files based on the p12, but that could be taken further to start from just the certs, or we could go even further and basically incorporate Simao's Makefile. That would be cool, actually. Also, it's annoying that in the current branch, I delete the old zips, add new ones, delete them, and add p12s. It should probably be refactored/rebased to just delete the old zips and add the p12s directly. I'm not fixing it right now in case there are other changes based on code review or testing with a real server.

Also: there should be more unit tests for various the combinations of client_auth.p12, api_gateway.url, and treehub.json. We should test that we prefer the p12 and URL if available.

codecov-io · 2019-06-26T09:10:22Z

Codecov Report

❗ No coverage uploaded for pull request base (master@383dfc3). Click here to learn what that means.
The diff coverage is 93.87%.

@@            Coverage Diff            @@
##             master    #1243   +/-   ##
=========================================
  Coverage          ?   79.33%           
=========================================
  Files             ?      171           
  Lines             ?    10160           
  Branches          ?        0           
=========================================
  Hits              ?     8060           
  Misses            ?     2100           
  Partials          ?        0

Impacted Files	Coverage Δ
src/sota_tools/treehub_server.h	`100% <ø> (ø)`
src/sota_tools/treehub_server.cc	`100% <100%> (ø)`
src/libaktualizr/bootstrap/bootstrap.cc	`57.77% <100%> (ø)`
src/sota_tools/authenticate.cc	`73.07% <100%> (ø)`
src/libaktualizr/bootstrap/bootstrap.h	`100% <100%> (ø)`
src/sota_tools/server_credentials.h	`76.47% <100%> (ø)`
src/sota_tools/server_credentials.cc	`94.04% <90%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 383dfc3...85deb96. Read the comment docs.

lbonn · 2019-06-26T09:11:00Z

Pushed a trivial fix for the debian testing ci

zabbal · 2019-06-26T09:51:21Z

src/sota_tools/authenticate_test.cc


 /* Authenticate with nothing (no auth).
 * Parse authentication information from treehub.json.
 * Parse images repository URL from a provided archive. */
 TEST(authenticate, good_cert_noauth_zip) {
+  // Authenticates with ssl_noauth_server on port 2443.


Just a minor nitpicking: would be better to use "tls" instead of "ssl" everywhere to be consistent with kTls and such.

mike-sul · 2019-07-03T07:30:26Z

src/sota_tools/authenticate_test.cc

 /* Authenticate with OAuth2.
 * Parse authentication information from treehub.json. */
 TEST(authenticate, good_zip) {
+  // Authenticates with the ATS portal to the SaaS instance.


What is "the ATS portal", is it connect.ota.here.com ?

Nope, that's the normal portal. https://app.atsgarage.com/ is the old portal that surprisingly still works but basically serves the same content.

mike-sul · 2019-07-03T07:35:15Z

src/sota_tools/authenticate_test.cc

  TreehubServer treehub;
-  int r = authenticate("", creds, treehub);
+  int r = authenticate("tests/fake_http_server/server.crt", creds, treehub);


Maybe, makes sense to pass "tests/fake_http_server/server.crt" as the test parameter ?

Sorry, where should it get passed from?

From corrsponding CMakeLists.txt

add_aktualizr_test(NAME SOURCES <> ARGS )

but, it's really not important, not sure if it's worth bothering

Seems like that's just moving the hardcoded value to a more obscure place. I'm not sure I see the benefit right now. That said, I'm interested in redoing this logic so that we can generate all the certs and such dynamically, and once that is done, something like what you are suggesting will probably be necessary.

Yeah, + for the dynamic certs generation, also using TEST_F/test fixtures/base class is an ideal for sharing the same context across multiple tests.

mike-sul · 2019-07-03T07:36:47Z

src/sota_tools/authenticate_test.cc


 /* Authenticate with nothing (no auth).
 * Parse authentication information from treehub.json.
 * Parse images repository URL from a provided archive. */
 TEST(authenticate, good_cert_noauth_zip) {
+  // Authenticates with ssl_noauth_server on port 2443.


Is this port number 2443 actually hard-coded value or it's a parameter or a configuration variable ?

It's hardcoded. It probably shouldn't be anymore. I'll look into that.

mike-sul · 2019-07-03T07:37:14Z

src/sota_tools/authenticate_test.cc

  boost::filesystem::path filepath = "tests/sota_tools/auth_test_noauth_good.zip";
  ServerCredentials creds(filepath);
  EXPECT_EQ(creds.GetMethod(), AuthMethod::kNone);
  TreehubServer treehub;
-  int r = authenticate("tests/fake_http_server/client.crt", creds, treehub);
+  int r = authenticate("tests/fake_http_server/server.crt", creds, treehub);


Maybe, makes sense to pass "tests/fake_http_server/server.crt" as the test parameter ?

mike-sul · 2019-07-03T07:42:16Z

src/sota_tools/authenticate_test.cc

  TreehubServer treehub;
-  int r = authenticate("", ServerCredentials(filepath), treehub);
+  int r = authenticate("", creds, treehub);
  EXPECT_EQ(0, r);
  CurlEasyWrapper curl_handle;
  curlEasySetoptWrapper(curl_handle.get(), CURLOPT_VERBOSE, 1);


Does it make sense to continue the test control flow if the authentication fails, if not then ASSERT_EQ should serve better here.

The EXPECT_EQ shouldn't hurt and it's not like we'll save a huge amount of test time if we fail earlier. We normally use EXPECT for everything unless there's a good reason not to. If there is consensus around changing that norm, I'm open to revising it, though.

No worries, I don't think that there is much practical benefits in it.

mike-sul · 2019-07-03T07:58:04Z

src/sota_tools/server_credentials.cc

+        method_ = AuthMethod::kTls;
+      } else if (strcmp(filename, "api_gateway.url") == 0) {
+        use_api_gateway = true;
+        ostree_server_ = readArchiveFile(a)->str();


I read this like ostree_server and api_gateway are the same entity.

Unfortunately, for backwards compatibility, we have to support the case where there is no API gateway for some period. Going forward, there should always be such a gateway, but if we don't have the URL in the credentials.zip (or there wasn't such a gateway deployed), we still need to be able to talk to treehub directly.

mike-sul · 2019-07-03T07:58:59Z

src/sota_tools/server_credentials.cc

-  try {
-    ptree pt;
+  if (use_api_gateway) {
+    repo_url_ = ostree_server_;


Now, ostree_server becomes repo_url :)

mike-sul

What would be the most efficient way to figure out design behind this different schema to authentication ?
How can I authenticate at and use the treehub server from a command line by utilizing ostree utility ?

pattivacek · 2019-07-12T07:39:07Z

What would be the most efficient way to figure out design behind this different schema to authentication ?

Some of the answers can be gleaned from https://github.com/advancedtelematic/aktualizr/blob/master/docs/provisioning-methods-and-credentialszip.adoc. What exactly are you looking for, though? Do you want a sequence diagram or a better explanation of the options?

How can I authenticate at and use the treehub server from a command line by utilizing ostree utility ?

You mean you want to use the ostree commandline tool? If so, I don't think you can. We are operating on OSTree objects directly and parsing them with glib ourselves to walk the object trees. (We do that to avoid a dependency on libostree.) Or is there something else you want to do?

mike-sul · 2019-07-12T08:45:59Z

What would be the most efficient way to figure out design behind this different schema to authentication ?

Some of the answers can be gleaned from https://github.com/advancedtelematic/aktualizr/blob/master/docs/provisioning-methods-and-credentialszip.adoc. What exactly are you looking for, though? Do you want a sequence diagram or a better explanation of the options?

How can I authenticate at and use the treehub server from a command line by utilizing ostree utility ?

You mean you want to use the ostree commandline tool? If so, I don't think you can. We are operating on OSTree objects directly and parsing them with glib ourselves to walk the object trees. (We do that to avoid a dependency on libostree.) Or is there something else you want to do?

I've already figured out what I wanted by debugging through the code. Thanks.

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

The old method required seperate client cert, key, and root CA in the credentials archive. It has been unused as far as I can tell for at least a year, if not two or more. The one test for it has been broken for a long time. That method has been replaced with mutual TLS support, which is what the backend is transitioning to. OAuth2 support will eventually get dropped on the server, but in the meantime we still need to support it for some transitional period. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Added new server certs to get this to work correctly. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Verify that it actually uses TLS but fails. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

This means they don't have to be hardcoded binary objects anymore. Even better would be to actually generate the p12 based on the certs as I'd originally planned but ran out of time for. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Used by tests/sota_tools/cert_generation/generate-zips.sh Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek · 2019-07-23T15:26:42Z

Should we merge this or wait until we can test with a "real" server? I had also considered doing some more work to make the test data more dynamically generated, but I didn't want to do it before I knew for sure that it worked with the final server implementation. I particularly wanted to get rid of the binary p12 files that I had to had to this PR in its current state.

lbonn · 2019-07-29T07:12:57Z

If it doesn't break something that's already working (which I think it's the case), we can probably merge it.

pattivacek · 2019-07-29T07:21:31Z

If it doesn't break something that's already working (which I think it's the case), we can probably merge it.

Yeah, fair enough. I don't want to have to keep rebasing this, and I'm pretty sure this is on the right track for the eventual integration with the real server implementation. There's still some improvements that could be made here, especially in the automation of the tests, but I think it's a good start.

pattivacek requested review from lbonn, eu-siemann and mike-sul June 25, 2019 15:47

pattivacek mentioned this pull request Jun 25, 2019

Feat/ota 2666/garage tools mutual tls advancedtelematic/meta-updater#532

Merged

zabbal reviewed Jun 26, 2019

View reviewed changes

mike-sul reviewed Jul 3, 2019

View reviewed changes

pattivacek force-pushed the feat/OTA-2666/garage-tools-mutual-tls branch from f1705ad to 3f9e8a6 Compare July 12, 2019 08:19

mike-sul closed this Jul 12, 2019

pattivacek reopened this Jul 12, 2019

pattivacek force-pushed the feat/OTA-2666/garage-tools-mutual-tls branch from 3f9e8a6 to 870db0a Compare July 15, 2019 10:20

pattivacek and others added 9 commits July 19, 2019 14:46

Refactor p12 parsing into a static function.

9d373d9

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Remove default URL and password for defunct server.

f0abed4

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Use p12 directly for TLS support in garage tools.

5b25552

Added new server certs to get this to work correctly. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Remove old client cert/key and use the server cert/key for all tests.

49535ba

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Fix bad_cert_zip test.

6074a24

Verify that it actually uses TLS but fails. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Generate the new zip files by scripts.

734d915

This means they don't have to be hardcoded binary objects anymore. Even better would be to actually generate the p12 based on the certs as I'd originally planned but ran out of time for. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Add zip in debian testing docker image

561132f

Used by tests/sota_tools/cert_generation/generate-zips.sh Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

Rename ssl_server to tls_server for consistency.

85deb96

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek force-pushed the feat/OTA-2666/garage-tools-mutual-tls branch from 870db0a to 85deb96 Compare July 19, 2019 12:47

lbonn approved these changes Jul 23, 2019

View reviewed changes

mike-sul approved these changes Jul 24, 2019

View reviewed changes

pattivacek merged commit f5668ae into master Jul 29, 2019

pattivacek deleted the feat/OTA-2666/garage-tools-mutual-tls branch July 29, 2019 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/ota 2666/garage tools mutual tls #1243

Feat/ota 2666/garage tools mutual tls #1243

pattivacek commented Jun 25, 2019 •

edited

Loading

codecov-io commented Jun 26, 2019 •

edited

Loading

lbonn commented Jun 26, 2019

zabbal Jun 26, 2019

pattivacek Jul 12, 2019

mike-sul Jul 3, 2019

pattivacek Jul 12, 2019

mike-sul Jul 3, 2019

pattivacek Jul 12, 2019

mike-sul Jul 12, 2019

mike-sul Jul 12, 2019

pattivacek Jul 12, 2019

mike-sul Jul 12, 2019

mike-sul Jul 3, 2019

pattivacek Jul 12, 2019

mike-sul Jul 3, 2019

mike-sul Jul 3, 2019

pattivacek Jul 12, 2019

mike-sul Jul 12, 2019

mike-sul Jul 3, 2019

pattivacek Jul 12, 2019

mike-sul Jul 3, 2019

mike-sul left a comment

pattivacek commented Jul 12, 2019

mike-sul commented Jul 12, 2019

pattivacek commented Jul 23, 2019 •

edited

Loading

lbonn commented Jul 29, 2019

pattivacek commented Jul 29, 2019

Feat/ota 2666/garage tools mutual tls #1243

Feat/ota 2666/garage tools mutual tls #1243

Conversation

pattivacek commented Jun 25, 2019 • edited Loading

codecov-io commented Jun 26, 2019 • edited Loading

Codecov Report

lbonn commented Jun 26, 2019

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

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

mike-sul left a comment

Choose a reason for hiding this comment

pattivacek commented Jul 12, 2019

mike-sul commented Jul 12, 2019

pattivacek commented Jul 23, 2019 • edited Loading

lbonn commented Jul 29, 2019

pattivacek commented Jul 29, 2019

pattivacek commented Jun 25, 2019 •

edited

Loading

codecov-io commented Jun 26, 2019 •

edited

Loading

pattivacek commented Jul 23, 2019 •

edited

Loading