Remove shared cred p12 from the archive after provisioning #1697
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1697 +/- ##
==========================================
- Coverage 76.57% 75.50% -1.07%
==========================================
Files 195 195
Lines 13353 13642 +289
==========================================
+ Hits 10225 10301 +76
- Misses 3128 3341 +213
Continue to review full report at Codecov.
|
| if (provision.mode == ProvisionMode::kDefault) { | ||
| provision.mode = provision.provision_path.empty() ? ProvisionMode::kDeviceCred : ProvisionMode::kSharedCred; | ||
| } else if (provision.mode == ProvisionMode::kSharedCredReuse) { | ||
| LOG_INFO << "Provisioning mode is set to reuse shared credentials. This should only be used for testing!"; |
There was a problem hiding this comment.
Would it make sense to fail early if provision.provision_path.empty()? Probably, doesn't matter, really
There was a problem hiding this comment.
You mean if using shared creds but the path is empty? Maybe, but it will fail during initialization anyway. Also, nothing would stop you from still importing device creds even if you claim you want to use shared creds (I think). And if you've already provisioned and you get the URL from elsewhere, technically you don't need to keep the provisioning path configured; it'll never get used again.
|
|
||
| struct archive *a_out = archive_write_new(); | ||
| if (a_out == nullptr) { | ||
| LOG_ERROR << "archive error: could not initialize archive object"; |
There was a problem hiding this comment.
Don't we need archive_read_free here?
There was a problem hiding this comment.
Yep, good call. I did some RAII cleanup to this code and it should be better now. Also rebased.
There was a problem hiding this comment.
Cool! I always forget that we have StructGuard for such cases.
|
Overall looks good! |
Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
pattivacek
force-pushed
the
feat/OTA-5018/rm-shared-creds
branch
from
476a3c4
to
a6758dc
Compare
It should never be needed again and presents a potential security risk if left available for an attacker who gains access to the device. Also remove treehub.json. It shouldn't be there in the first place, but in the distant past it was often accidentally left there. Improved some RAII with archive structs, too. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
The default is still to decide based on whether provision_path is empty, but you can also make your preferences explicit. There is also a new option: kSharedCredReuse, which will skip the removal of the shared credential p12 from the archive. This is intended to be used only for testing. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
Not strictly necessary but slightly safer. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
pattivacek
force-pushed
the
feat/OTA-5018/rm-shared-creds
branch
from
a6758dc
to
c254ff8
Compare
| @@ -181,6 +181,16 @@ void Initializer::initTlsCreds() { | |||
| throw Error("Failed to configure HTTP client with device credentials."); | |||
| } | |||
|
|
|||
| // Remove shared provisioning credentials from the archive; we have no more | |||
| // use for them. | |||
| Utils::removeFileFromArchive(config_.provision_path, "autoprov_credentials.p12"); | |||
There was a problem hiding this comment.
Should we also add this call to try-catch block
There was a problem hiding this comment.
I don't think so; it should succeed. treehub.json is in a try-catch because normally it should fail; that file should not be there.
| boost::filesystem::rename(outfile.Path(), archive_path); | ||
| } else { | ||
| boost::filesystem::remove(outfile.Path()); | ||
| throw std::runtime_error("Requested file not found in archive!"); |
There was a problem hiding this comment.
I would prefer to return bool value in case of entry wasn't found in the archive, instead of throwing an exception
There was a problem hiding this comment.
To be honest, I just copied the style of the existing functions readFileFromArchive and writeArchive. I think consistency is good, and I don't see a compelling reason to prefer error codes over exceptions. Can you explain your preference?
|
I don't understand why the github CI job is failing; I fixed the problem and it works on my laptop. The line it is quoting as failing has been fixed; you can compare it with the actual code here. |
It should never be needed again and presents a potential security risk if left available for an attacker who gains access to the device.
Also remove
treehub.json. It shouldn't be there in the first place, but in the distant past it was often accidentally left there.There is now an explicit config option to prevent this behavior if you don't want it, which should only be used for tests. Set
provision.mode = "SharedCredReuse"to use that.