fix: fail loudly when uuid cannot resolve device identity#31
Merged
Conversation
`getClientId()` shells out to `uuid` on PATH — a deliberate abstraction, since
each platform supplies its own (Yocto via the fb-uuid recipe, the factory
container via a shim reading /data/.device-uuid). But it swallowed every
failure: the IOException was caught and printStackTrace()'d, a non-zero exit
was never checked, stderr was discarded, and it returned null.
Null then reached sign(), which failed with:
NullPointerException: Cannot invoke "String.getBytes(String)"
because "plainText" is null
That is a RuntimeException, so the surrounding catch (GeneralSecurityException
| IOException) missed it and the nucleus reported only "Caught exception while
running provisioning plugin" — while the shim's own diagnosis ("device not
provisioned") had already been thrown away with its stderr. Three layers each
discarded the answer.
Check the exit code, capture stderr, and throw
DeviceProvisioningRuntimeException with the command's own message. Move the
getClientId() call out of the signing try-block: failing to resolve identity is
not a signing error and must not be reported as one.
Also fix the logger, which was constructed for MqttConnectionHelper.class, and
add tests — the class had none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
getClientId()resolves the device UUID by runninguuidon PATH. The indirection is deliberate — each platform supplies its own (Yocto via the fb-uuid recipe, the factory container via a shim reading/data/.device-uuid) and the plugin must not know any platform's on-disk layout. That part is fine.What isn't: it swallowed every failure. The
IOExceptionwas caught andprintStackTrace()'d, a non-zero exit was never checked, stderr was discarded, and it returnednull.Null then reached
sign():NullPointerExceptionis aRuntimeException, so the surroundingcatch (GeneralSecurityException | IOException)missed it, and the nucleus logged only "Caught exception while running provisioning plugin. Moving on to run Greengrass without provisioning." Meanwhile the shim had already printed the actual answer —uuid: /data/.device-uuid missing — device not provisioned— to a stderr nobody read.Three layers each discarded the diagnosis. Hit for real while bringing up a cloud-hosted Factbird Factory; it presents as a crypto bug and isn't one.
Changes
DeviceProvisioningRuntimeExceptioncarrying the command's own message.getClientId()call out of the signing try-block — failing to resolve identity is not a signing error and shouldn't be reported as one.MqttConnectionHelper.class.DeviceIdentityHelperTest— the class had no coverage. Theexits_nonzerocase reproduces the failure above and asserts the diagnosis survives.A package-private
getClientId(String[])overload lets the tests point at a stub instead of depending on PATH. Public behaviour is unchanged.