Add `aziot check` #83

daprilik · 2020-12-08T18:41:21Z

Adds a new aziot check subcommand to aziot, modeled after iotedge check.

Should be reviewed in conjunction with the upstream PR at Azure/iotedge#4153

Please note that the aziot check implementation is not a straight copy-paste from iotedge check, and while the two implementations have a lot in common, aziot check's underlying check framework has been substantially refactored, simplified, and modernized. Notable changes include the use of async_trait on the Check trait, the use of serde_erased to simplify checker serialization, and removing all the Windows console specific text formatting code.

While the internals of aziot check are quite different from iotedge check, I've made sure that the user-facing text and json output formats have remained exactly the same between the two tools.

One notable addition to aziot check is the new json-stream output format, which will enable iotedge check to invoke iotedge check as a sub-process. Aside from streamlining check flow (users won't have to invoke both check commands separately), the additional_info payload will be used to share certain bits of provisioning-specific information with iotedge that it wouldn't otherwise have access to (e.g: the iothub_hostname configuration option used in the host connectivity checks in iotedge check is part of the identityd config, which iotedge should not be able to access directly).

This PR should be reviewed along two axes:

The aziot check subcommand, framework, and implementation quality
The actual checks themselves

The checks are divided into two categories:

Checks ported over from iotedge check (due to certain functionality being moved down the stack into aziot)
- host_connect_dps_endpoint
- host_local_time
- hostname
- identity_certificate_expiry
- well_formed_configs
New checks specific to the new aziot architecture
- certs_preloaded
- dameons_running

While the primary goal of this PR is to get aziot check "up and running" in an effort to fix iotedge check, if there are any checks that I may have missed that are "critical" to merge as part of this initial PR, please let me know.

This PR will be un-drafted once the corresponding iotedge check PR has been submitted, as there's a non-zero chance the json-streaming output format will require changes as part of the integration work.

massand

LGTM so far. Few comments

aziot/src/internal/check/additional_info.rs

massand · 2020-12-10T08:03:15Z

aziot/src/internal/check/checks/host_local_time.rs

+        shared: &CheckerShared,
+        _cache: &mut CheckerCache,
+    ) -> Result<CheckResult> {
+        // TODO: check against parent time in nested edge scenarios


IS doesn't have any knowledge of nested edge so far. Edge does have awareness of the parent_hostname configuration and hence nested edge.

Reviewing my notes, I actually have no idea why I left this TODO...
The original host_local_time check doesn't even do that. https://github.com/Azure/iotedge/blob/release/1.0.10/edgelet/iotedge/src/check/checks/host_local_time.rs

Ahh, well, now I remember why I left this TODO... I accidentally liked to an older version of the check from before nested-edge. The latest version does include some extra logic in nested-edge scenarios:
https://github.com/Azure/iotedge/blob/release/1.2/edgelet/iotedge/src/check/checks/host_local_time.rs#L41-L46

So, what should we do to account for that extra logic?

The nested check seems strange because the root node of the nested edge topology would still need to be in sync with NTP (to connect to IoT Hub). All child nodes down the chain would then be transitively in sync with NTP. IS does not have the concept of parent_hostname because the parent hostname is the iothub_hostname. So, the NTP check is good enough for both cases in IS and you can remove the parent_hostname time check altogether.

Awesome, that's what I like to hear 😄

We need both the NTP check and the HTTP Date header check in IS. If IS (and thus the check) doesn't know it's running in a nested scenario, the check might have to try NTP and then fall back to the HTTP Date header method.

@massand I don't understand what you mean by "all child nodes down the chain would then be transitively in sync with NTP."

aziot/src/internal/check/checks/host_local_time.rs

aziot/src/internal/check/checks/mod.rs

still a lot of room for improvement, but this seems like a reasonable foundation.

unfortunately, anyhow doesn't supporting using the `backtrace` crate for backtraces on stable, relying on the currently unstable std::backtrace instead.

this is basically a straight copy/paste, aside from the removal of Failure for error handling.

oh boy, I sure did go down a rabbit hole trying to support "nested" checks. let me tell ya - it's not trivial, and will require substantial changes to the code in `check.rs`. I'm not too keen on doing that right now, so I'm pushing it away for later. also, I realized that its' probably a _bad_ idea to rely on the cert service actually running to fetch certificates. as such, checks will now directly read certificates from disk.

wow there was _very_ little documentation on how to perform a TLS handshake with hyper_openssl. Turns out you have to treat the HttpsConnector as a hyper::Service, and then `.call()` it, thereby getting back a handshake future. There are _zero_ docs which describe this, and the only reason I figured it out was by crossreferencing the hyper_openssl implementation with hyper_tls.

this will un-break some of the iotedge check checks.

enables 'iotedge check-list' to list aziot checks as well.

this new crate is consumed by `iotedge check`, and ensures the two tools stay in-sync.

aziot/aziot-check-common/src/lib.rs

massand

aziot/Cargo.toml

aziot/src/main.rs

aziot/src/check.rs

aziot/src/internal/check/mod.rs

aziot/src/internal/common.rs

identity/aziot-identityd-config/src/lib.rs

mini-sntp/src/lib.rs

- removed internal check::prelude - removed some unused imports found using `cargo udeps` - other tweaks

aziot/src/internal/check/checks/host_connect_iothub.rs

aziot/src/internal/check/checks/identity_certificate_expiry.rs

cert/aziot-certd-config/src/util.rs

aziot/src/internal/check/mod.rs

aziot/src/internal/check/checks/mod.rs

this will require an update to the upstream iotedge check PR as well

aziot/src/internal/check/checks/identity_certificate_expiry.rs

aziot/src/internal/check/checks/host_connect_iothub.rs

aziot/src/internal/check/checks/cert_expiry.rs

Updates `iotedge check` to work with the new underlying `aziot` architecture. This PR builds on-top of the functionality introduced in the downstream `aziot` PR at Azure/iot-identity-service#83 * * * In a nutshell, many of the checks that used to be in `iotedge check` have been ported over into the new `aziot check` tool. Please see the associated downstream PR for more details. That being said, we don't want to make users invoke multiple `check` commands, so this PR also includes functionality whereby `iotedge` will shell-out to `aziot` when running `check` and `check-list`, transparently displaying `aziot`'s check results alongside `iotedge`-specific checks. As with the downstream PR, this PR should be reviewed along two axes: 1. Refactoring work around the actual `iotedge check/check-list` subcommands 2. The actual checks themselves (i.e: confirming that all removed checks have indeed been migrated / aren't applicable anymore). I would **really appreciate it** if the reviewers actually pulled these changes locally and tested them out, as subtle things such as check output formatting and/or checking check pass/failure cases aren't easy to spot by looking at the code alone.

daprilik requested review from massand and arsing December 8, 2020 18:41

massand reviewed Dec 10, 2020

View reviewed changes

daprilik added 25 commits December 18, 2020 13:46

rough framework for aziot check

8390c75

still a lot of room for improvement, but this seems like a reasonable foundation.

improve project topology + add streaming-json output

e890bb0

swap out dummy check for unimplemented well_formed_configs check

5bd64a3

integrate anyhow

cb66c2e

unfortunately, anyhow doesn't supporting using the `backtrace` crate for backtraces on stable, relying on the currently unstable std::backtrace instead.

add well formed config checks

cb9d16c

r u s t f m t

1a420d1

add hostname check

7ae2011

port mini-sntp to aziot

fafc435

this is basically a straight copy/paste, aside from the removal of Failure for error handling.

add host_local_time check

afe3361

add identity_certificate_expiry check

e06f1eb

augment certs_preloaded check to validate nested ids

2c3d5bc

fine-grained skipping based on which configs are valid

0f93bcc

add iothub_hostname to additional_info

aa62b6a

this will un-break some of the iotedge check checks.

add daemon running checks

6e4731b

add copyright headers

d043b20

fix mini-sntp packaging

44d297e

add MPL-2 support to third-party-notices.sh

81d14bf

minor PR fixes

603b997

add iotedge connectivity checks

b52a035

add check-list JSON output option

c919fca

enables 'iotedge check-list' to list aziot checks as well.

Merge branch 'main' into aziot-check

74f32a1

remove direct dep on cerd from aziot

a6e535a

remove direct dep on identityd from aziot

be1c1db

daprilik force-pushed the aziot-check branch from c2b1d87 to be1c1db Compare December 18, 2020 19:34

add more missing copyright headers

2ddb9a9

extract aziot-check JSON API into separate crate

1ac11e7

this new crate is consumed by `iotedge check`, and ensures the two tools stay in-sync.

daprilik force-pushed the aziot-check branch from 731c130 to 1ac11e7 Compare December 23, 2020 22:09

massand reviewed Jan 4, 2021

View reviewed changes

aziot/aziot-check-common/src/lib.rs Outdated Show resolved Hide resolved

massand approved these changes Jan 4, 2021

View reviewed changes

daprilik added 3 commits January 4, 2021 16:11

CheckOuput -> CheckOutput

457313c

Merge branch 'main' into aziot-check

98bb8a4

clippy *shakes fist*

f6735c9

arsing requested changes Jan 5, 2021

View reviewed changes

fix PR comments

4fd3e53

daprilik requested a review from arsing January 5, 2021 21:51

more PR comments

5e7f3bd

- removed internal check::prelude - removed some unused imports found using `cargo udeps` - other tweaks

daprilik force-pushed the aziot-check branch from 2837113 to 5e7f3bd Compare January 5, 2021 22:13

arsing requested changes Jan 5, 2021

View reviewed changes

fix cerd perms + partial fix to host_connect_iothub

c843ef7

arsing reviewed Jan 6, 2021

View reviewed changes

aziot/src/internal/check/checks/mod.rs Outdated Show resolved Hide resolved

daprilik added 3 commits January 6, 2021 10:42

fix fat fingered "fix"

6ae866e

fix identity_cert_expiry check when using dynamically issued certs

6d5dd05

add iothub-hostname flag

1d5749d

this will require an update to the upstream iotedge check PR as well

arsing requested changes Jan 7, 2021

View reviewed changes

daprilik added 2 commits January 7, 2021 13:46

adress yet more PR comment

221d7f7

add localca, est identity + bootstrap cert checks

6d465ad

daprilik requested a review from arsing January 7, 2021 22:50

arsing reviewed Jan 7, 2021

View reviewed changes

aziot/src/internal/check/checks/cert_expiry.rs Outdated Show resolved Hide resolved

arsing approved these changes Jan 7, 2021

View reviewed changes

phrasing

ed70446

daprilik added the ready-to-merge label Jan 7, 2021

Merge branch 'main' into aziot-check

e01315e

kodiakhq bot merged commit a344cfa into Azure:main Jan 7, 2021

daprilik deleted the aziot-check branch January 22, 2021 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `aziot check` #83

Add `aziot check` #83

daprilik commented Dec 8, 2020 •

edited

Loading

massand left a comment

massand Dec 10, 2020

daprilik Dec 11, 2020 •

edited

Loading

daprilik Dec 14, 2020

massand Dec 14, 2020

daprilik Dec 14, 2020

arsing Dec 14, 2020

massand left a comment

Add aziot check #83

Add aziot check #83

Conversation

daprilik commented Dec 8, 2020 • edited Loading

massand left a comment

Choose a reason for hiding this comment

massand Dec 10, 2020

Choose a reason for hiding this comment

daprilik Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

daprilik Dec 14, 2020

Choose a reason for hiding this comment

massand Dec 14, 2020

Choose a reason for hiding this comment

daprilik Dec 14, 2020

Choose a reason for hiding this comment

arsing Dec 14, 2020

Choose a reason for hiding this comment

massand left a comment

Choose a reason for hiding this comment

Add `aziot check` #83

Add `aziot check` #83

daprilik commented Dec 8, 2020 •

edited

Loading

daprilik Dec 11, 2020 •

edited

Loading