Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

aktualizr-lite: Support using TLS keys #1237

Merged
merged 2 commits into from
Jun 25, 2019

Conversation

doanac
Copy link
Collaborator

@doanac doanac commented Jun 18, 2019

This is a bit of an edge case, but I've found it useful to run
devices in the lite mode, but to still restrict who can access
the tuf/treehub repos via TLS.

Signed-off-by: Andy Doan [email protected]

@codecov-io
Copy link

codecov-io commented Jun 18, 2019

Codecov Report

Merging #1237 into master will increase coverage by 0.11%.
The diff coverage is 87.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1237      +/-   ##
==========================================
+ Coverage   79.27%   79.38%   +0.11%     
==========================================
  Files         170      170              
  Lines       10140    10146       +6     
==========================================
+ Hits         8038     8054      +16     
+ Misses       2102     2092      -10
Impacted Files Coverage Δ
src/libaktualizr/primary/initializer.h 100% <ø> (ø) ⬆️
src/libaktualizr/primary/sotauptaneclient.h 100% <ø> (ø) ⬆️
src/libaktualizr/primary/initializer.cc 80% <50%> (-0.96%) ⬇️
src/libaktualizr/primary/sotauptaneclient.cc 92.48% <75%> (+0.31%) ⬆️
src/aktualizr_lite/main.cc 68.78% <95.65%> (+3.57%) ⬆️
src/libaktualizr-posix/ipuptanesecondary.cc 82.66% <0%> (-4%) ⬇️
src/aktualizr_repo/main.cc 76.92% <0%> (-2.46%) ⬇️
src/aktualizr_primary/secondary.cc 86.3% <0%> (-1.37%) ⬇️
src/libaktualizr/uptane/imagesrepository.cc 92.59% <0%> (ø) ⬆️
... and 4 more

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 c9826b1...4534d50. Read the comment docs.

@doanac doanac changed the title aktualizr-lite: Support using TLS keys WIP: aktualizr-lite: Support using TLS keys Jun 18, 2019
@doanac doanac changed the title WIP: aktualizr-lite: Support using TLS keys aktualizr-lite: Support using TLS keys Jun 18, 2019
Copy link
Contributor

@zabbal zabbal left a comment

Choose a reason for hiding this comment

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

Could you expand on the context a bit: this patch effectively always hides this warning. Could there be some setup/configuration where we do not want this? Or can this always be ignored?

hwid = Utils::getHostname();
}
if (serial.empty()) {
serial = Utils::getHostname();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if I understand this logic for generating a serial. Reusing hostname seems like a problem. Serials should be unique, at least within a namespace, and hostnames are rather likely to be non-unique. Our standard technique is to use the key ID of the ECU's Uptane key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem there is that in "lite" mode there is no uptane key. I guess I could grab the UUID from the client.pem and use that since it would be unique? However, aktualizr-lite doesn't require TLS, so that might not be there either. I guess I could just generate a UUID and use that?

@@ -205,7 +205,7 @@ InitRetCode Initializer::initEcuRegister() {
Initializer::Initializer(
const ProvisionConfig& config_in, std::shared_ptr<INvStorage> storage_in,
std::shared_ptr<HttpInterface> http_client_in, KeyManager& keys_in,
const std::map<Uptane::EcuSerial, std::shared_ptr<Uptane::SecondaryInterface> >& secondary_info_in)
const std::map<Uptane::EcuSerial, std::shared_ptr<Uptane::SecondaryInterface> >& secondary_info_in, bool lite_mode)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't particularly like punching holes in our initialization logic, but I haven't yet thought of a better way that isn't just duplicating code. But can we at least change the name? lite_mode seems rather hard to understand without already knowing the context. Perhaps tls_only would be better? I do like that it defaults to false, though!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a bit uneasy about this as well. Adding conditional logic into a security related flow could cause a subtle bug some day. I think there are two other options here:

  1. Keep this patch out-of-tree.
    Its kind of specific to something we are doing. I can understand if this isn't interesting to you guys.

  2. Have aktualizr-lite use the SotatUptaneClient constructor rather than calling newDefaultClient
    If we changed things to this, then I could actually just call what I needed directly, keys_.copyCertsToCurl(*http_client_);

I haven't tried out option 2 but I think that might work better here and not leak details of the lite stuff into sotauptaneclient. Sound reasonable?

@doanac
Copy link
Collaborator Author

doanac commented Jun 21, 2019

I think this new version is a bit better. I can understand if you guys don't want the 2nd commit "Set a default ECU Serial", but I think the first commit is probably worth merging.

@pattivacek
Copy link
Collaborator

Generating a random serial is fine if you have nothing else to go off. But I'd still prefer if we could rename lite_mode to something like tls_only in order to make it more descriptive.

@doanac
Copy link
Collaborator Author

doanac commented Jun 24, 2019

@patrickvacek - sorry - i need to back out the changes to the sotauptaneclient and initializr. The latest revision doens't call those paths.

doanac added 2 commits June 24, 2019 09:09
This is a bit of an edge case, but I've found it useful to run
devices in the lite mode, but to still restrict who can access
the tuf/treehub repos via TLS.

Signed-off-by: Andy Doan <[email protected]>
You get a warning message every time this tool is run when there is
no ecu serial configured:

  warning: Could not find primary ecu serial, defaulting to empty serial: unknown error

This detects the situation and will give it a "good enough" default.

Signed-off-by: Andy Doan <[email protected]>
@doanac
Copy link
Collaborator Author

doanac commented Jun 24, 2019

This new diff should be better.

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

Seems fine with me! Not sure why Travis is claiming it's still pending... looks to me like it passed.

@pattivacek pattivacek merged commit ca4d71c into advancedtelematic:master Jun 25, 2019
@doanac doanac deleted the lite-with-tls branch July 8, 2019 04:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants