Skip to content

Conversation

rouming
Copy link
Contributor

@rouming rouming commented Aug 17, 2023

The aim of this PR is to make LOC more secure. PR does the following:

  1. Supports requesting controller certificates from the LOC
  2. Decrypts device config, which will be returned by the LOC as an AuthContainer envelope.

For the 1. the main change lies in reusing LOC URL as a fallback in case of error. The HTTP request and certs format envelope is unchanged.

For the 2. AuthContainer envelope was extended in this PR lf-edge/eve-api#14 with cipher members, which indicate that AuthContainer has be to be first decrypted and then signature should be verified.

@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.02% ⚠️

Comparison is base (42add92) 20.75% compared to head (11aa8bc) 20.74%.
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3414      +/-   ##
==========================================
- Coverage   20.75%   20.74%   -0.02%     
==========================================
  Files         196      196              
  Lines       43840    43861      +21     
==========================================
- Hits         9100     9098       -2     
- Misses      34065    34087      +22     
- Partials      675      676       +1     
Files Changed Coverage Δ
pkg/pillar/cmd/domainmgr/domainmgr.go 0.45% <0.00%> (ø)
pkg/pillar/cmd/zedagent/attesttask.go 2.63% <0.00%> (ø)
pkg/pillar/cmd/zedagent/handlecertconfig.go 0.00% <0.00%> (ø)
pkg/pillar/cmd/zedagent/handlecipherconfig.go 0.00% <0.00%> (ø)
pkg/pillar/cmd/zedagent/handleconfig.go 0.00% <0.00%> (ø)
pkg/pillar/dpcreconciler/linux.go 70.58% <0.00%> (ø)
pkg/pillar/types/cipherinfotypes.go 0.00% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rouming rouming force-pushed the encrypted-authcontainer branch from 92b297c to 6a1c394 Compare August 17, 2023 14:14
Just autogenerated stuff after updating the eve-api and calling:

  make proto-vendor

Nothing to look at.

Signed-off-by: Roman Penyaev <[email protected]>
According to the recent proto API changes from the /eve-api
repo all crypto related structures were moved from the 'config'
package to the 'common' package.

This patch reflects those API changes.

Signed-off-by: Roman Penyaev <[email protected]>
@rouming rouming force-pushed the encrypted-authcontainer branch from 6a1c394 to 264e1bf Compare August 18, 2023 15:16
@rouming rouming changed the title [WIP] LOC: support controller certs update and encrypted config LOC: support controller certs update and encrypted config Aug 18, 2023
@eriknordmark
Copy link
Contributor

Given the zconfig -> zcommon move in eve-api we need to get the controllers to pick that up as well. It would make sense to do that now so that it doesn't become a question and obstacle when the controllers need to pick up some new functionality from the API later.

Comment on lines +336 to +337
// If main controller is unavailable (@false is returned), the next set of
// attempts is to fallback to retrieve certs from the LOC.
Copy link
Contributor

Choose a reason for hiding this comment

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

For the EdgeDevConfig we try to fetch from both locations and compare the timestamp to make sure we don't accidentally accept an older config. Do we need the same type of checks for the certs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this makes a lot of sense to add this check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively save the certs we receive from both but that would require retry logic since today EVE assumes there is one controller signing certificate and one controller/ECDH certificate for object encryption.

Given that, unlike the EdgeDevConfig, we don't have a timestamp in the payload returned by the certificate API, we can't add a simple compare. So this probably requires some more thought. Could wrap up this PR first if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eriknordmark PEM certificates we are using have the following fields:

NotBefore, NotAfter [time](https://pkg.go.dev/time).[Time](https://pkg.go.dev/time#Time) // Validity bounds.

(https://pkg.go.dev/crypto/x509#Certificate)

Assuming we always moving forward with certificates, we apply a new certificate only if it is not older:

if newCert.NotBefore <= oldCert.NotBefore:
       reject

This still gives us freedom to correct mistakes if new certificates were issues incorrectly by keeping the NotAfter date same and moving the NotBefore forward.

This change is relatively small and can be done in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very good suggestion.

docs/CONFIG.md Outdated
In such cases it is required to deliver the initial, also known as *bootstrap*, device configuration off-line. The recommended method (over some legacy mechanisms described below) is to prepare a single-use EVE installation medium, carrying the bootstrap config for the target device inside the EVE partition.

The bootstrap configuration is modeled using the same Protobuf message that models device configuration delivered on-line: [EdgeDevConfig](https://github.com/lf-edge/eve-api/tree/main/proto/config/devconfig.proto). Just like in the on-line config delivery, an instance of `EdgeDevConfig` is protobuf-encoded and put into the `AuthContainer` envelope alongside a signature (see [OBJECT-SIGNING.md](https://github.com/lf-edge/eve-api/tree/main/OBJECT-SIGNING.md)). For the off-line signature verification (that must pass for the device to accept the configuration), it is necessary to further wrap `AuthContainer` together with the signing and all intermediary controller certificates inside [BootstrapConfig](https://github.com/lf-edge/eve-api/tree/main/proto/config/devconfig.proto). Finally, `BootstrapConfig` should be protobuf-encoded and written in this binary format into the EVE partition of a single-use EVE installer as `bootstrap-config.pb`. During the EVE installation, this file is copied into the CONFIG partition.
The bootstrap configuration is modeled using the same Protobuf message that models device configuration delivered on-line: [EdgeDevConfig](https://github.com/lf-edge/eve-api/tree/main/proto/config/devconfig.proto). Just like in the on-line config delivery, an instance of `EdgeDevConfig` is protobuf-encoded and put into the `AuthContainer` envelope alongside a signature or possibly encrypted payload (see [OBJECT-SIGNING.md](https://github.com/lf-edge/eve-api/tree/main/OBJECT-SIGNING.md)). For the off-line signature verification (that must pass for the device to accept the configuration), it is necessary to further wrap `AuthContainer` together with the signing and all intermediary controller certificates inside [BootstrapConfig](https://github.com/lf-edge/eve-api/tree/main/proto/config/devconfig.proto). Finally, `BootstrapConfig` should be protobuf-encoded and written in this binary format into the EVE partition of a single-use EVE installer as `bootstrap-config.pb`. During the EVE installation, this file is copied into the CONFIG partition.
Copy link
Contributor

Choose a reason for hiding this comment

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

an instance of EdgeDevConfig is protobuf-encoded and put into the AuthContainer envelope alongside a signature or possibly encrypted payload

I would suggest:
an instance of EdgeDevConfig is protobuf-encoded, signed and possibly even encrypted inside AuthContainer.

The recent /eve-api modifies the AuthContainer in such a way,
that payload can come encrypted (see OBJECT-SIGNING.md in the
/eve-api repo for detail). This patch adds a few words about
the possibly encrypted `AuthContainer`.

Signed-off-by: Roman Penyaev <[email protected]>
…ails

Retrieve certs from LOC if main controller returns an error and LOC has
a valid configuration.

Signed-off-by: Roman Penyaev <[email protected]>
In the next patches the `RemoveAndVerifyAuthContainer`
function will be responsible for the decrypting content
of the `AuthContainer`. This patch only changes the API,
no functional changes.

Signed-off-by: Roman Penyaev <[email protected]>
Payload of the `AuthContainer` can be not only signed, but also
encrypted. The patch implements decryption reusing the crypto
package and corresponding `DecryptCipherBlock`.

Signed-off-by: Roman Penyaev <[email protected]>
Both publication and subscription pubsub interfaces have
`Get` method which returns an interface by a key.
Sometimes the only operations which is needed is a lookup
by a key having either subscription or publication.

In order to abstract the getter functionality a new `Getter`
interface is created.

Signed-off-by: Roman Penyaev <[email protected]>
In the future patches `DecryptCipherContext` will be used for
decrypting config having Publication instead of Subscription.
Inside decrypting functions the only method of the Subscription
interface is the `Get` method. In order to use both Publication
and Subscription the `Getter` interface was created.

In this patch the `Subscription` is replaced on `Getter` interface.

Signed-off-by: Roman Penyaev <[email protected]>
The `PubSub` prefix reflects `Getter` interface better.

Signed-off-by: Roman Penyaev <[email protected]>
…ntainer

Config can come encrypted (LOC). For that particular case we create
and pass cipher decrypt context.

This patch enables config decryption.

Signed-off-by: Roman Penyaev <[email protected]>
No functional changes, just remove unused code.

Signed-off-by: Roman Penyaev <[email protected]>
@rouming rouming force-pushed the encrypted-authcontainer branch from 264e1bf to 11aa8bc Compare August 22, 2023 11:48
@rouming
Copy link
Contributor Author

rouming commented Aug 22, 2023

Difference to the previous version:

  • Docs tweaks (@milan-zededa suggestion)
  • Remove crc32 calculation for making the CipherBlockID unique - actually not needed if we don't publish that (@eriknordmark suggestion)
  • Remove unused code (last commit)

@eriknordmark eriknordmark merged commit ed4fe09 into lf-edge:master Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants