You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2021/07/27 00:35:48 UTC
[GitHub] [mynewt-artifact] WeekendSuperhero opened a new pull request #32: Support Hardware Encryption is isEncrypted function
WeekendSuperhero opened a new pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] WeekendSuperhero commented on pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
WeekendSuperhero commented on pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#issuecomment-896474247
> @WeekendSuperhero Please squash all those commits with a useful commit message.
I can squash and merge with a meaningful message when this PR is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] agross-korg commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r684355654
##########
File path: image/image.go
##########
@@ -790,7 +790,18 @@ func DecryptHwFull(img Image, secret []byte) (Image, error) {
return img, nil
}
-// IsEncrypted indicates whether an image's "encrypted" flag is set.
+// IsEncrypted indicates whether an image's "encrypted" flag is set or hw encryption is used.
func (img *Image) IsEncrypted() bool {
- return img.Header.Flags&IMAGE_F_ENCRYPTED != 0
+ enc := false
+ if img.Header.Flags&IMAGE_F_ENCRYPTED != 0 {
+ enc = true
+ } else {
+ for _, tlv := range img.ProtTlvs {
+ if tlv.Header.Type&IMAGE_TLV_SECRET_ID_LEGACY != 0 || tlv.Header.Type&IMAGE_TLV_SECRET_ID != 0 {
+ enc = true
+ }
+ }
Review comment:
@ccollins476ad for the hardware encrypted images, we weren't setting the encrypted flag. For those images we used a pair of TLVs describing the hw key offset to be used and a nonce.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] WeekendSuperhero commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
WeekendSuperhero commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r691500137
##########
File path: image/image.go
##########
@@ -813,3 +813,25 @@ func (img *Image) Bin() ([]byte, error) {
return b.Bytes(), nil
}
+
+// HasEncryptionPayload indicates whether an image's contains a HW encryption payload.
+func (img *Image) HasEncryptionPayload() bool {
+ enc := false
+ res, _ := img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID_LEGACY)
+ if res != nil {
+ nonce, _ := img.FindAllUniqueTlv(IMAGE_TLV_AES_NONCE_LEGACY)
+ if nonce != nil {
+ enc = true
+ }
+
+ }
Review comment:
no. I've optimized the solution using your feedback
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] utzig commented on pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
utzig commented on pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#issuecomment-898518274
The code itself looks like it's doing the right thing, but I wonder about the semantics, because having an encrypted image is different from having a payload (TLV) with data for a HW encryption engine (which also happens to have an encrypted image but not one MCUboot would be able to decrypt, for example), so someone else using this library (assuming there is anyone) would not have it's "app"/"lib" break. Have you considered creating a `HasEncryptionPayload` function (or something similar) and then from the place you need this functionality doing `IsEncrypted() || HasEncryptionPayload()`? Would that solve the problem?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] ccollins476ad commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
ccollins476ad commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r679300433
##########
File path: image/image.go
##########
@@ -790,7 +790,18 @@ func DecryptHwFull(img Image, secret []byte) (Image, error) {
return img, nil
}
-// IsEncrypted indicates whether an image's "encrypted" flag is set.
+// IsEncrypted indicates whether an image's "encrypted" flag is set or hw encryption is used.
func (img *Image) IsEncrypted() bool {
- return img.Header.Flags&IMAGE_F_ENCRYPTED != 0
+ enc := false
+ if img.Header.Flags&IMAGE_F_ENCRYPTED != 0 {
+ enc = true
+ } else {
+ for _, tlv := range img.ProtTlvs {
+ if tlv.Header.Type&IMAGE_TLV_SECRET_ID_LEGACY != 0 || tlv.Header.Type&IMAGE_TLV_SECRET_ID != 0 {
+ enc = true
+ }
+ }
Review comment:
`tlv.Header.Type` is a regular integer, not a bitmap. So you should compare it with the constant directly using `==` rather than `&`.
But I think it would be easier to just use the "find" functions that already exist. That suggestion is up to you though.
Are you sure this doesn't break anything? I remember there being some tricky image types that used encryption, but whose encrypted flag was unset. I recall it being important that these images appeared unencrypted during some parts of the image production process. It has been a long time since I've looked at any of this, so I could definitely be wrong. If you say it works, I'll take your word for it.
```suggestion
if img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID_LEGACY) != nil || img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID) != nil {
enc = true
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] WeekendSuperhero commented on pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
WeekendSuperhero commented on pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#issuecomment-898914745
> The code itself looks like it's doing the right thing, but I wonder about the semantics, because having an encrypted image is different from having a payload (TLV) with data for a HW encryption engine (which also happens to have an encrypted image but not one MCUboot would be able to decrypt, for example), so someone else using this library (assuming there is anyone) would not have it's "app"/"lib" break. Have you considered creating a `HasEncryptionPayload` function (or something similar) and then from the place you need this functionality doing `IsEncrypted() || HasEncryptionPayload()`? Would that solve the problem?
That would resolve the problem, I have to check the code that uses the isEncrypted() flag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] WeekendSuperhero commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
WeekendSuperhero commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r691485532
##########
File path: image/image.go
##########
@@ -813,3 +813,25 @@ func (img *Image) Bin() ([]byte, error) {
return b.Bytes(), nil
}
+
+// HasEncryptionPayload indicates whether an image's contains a HW encryption payload.
+func (img *Image) HasEncryptionPayload() bool {
+ enc := false
+ res, _ := img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID_LEGACY)
+ if res != nil {
+ nonce, _ := img.FindAllUniqueTlv(IMAGE_TLV_AES_NONCE_LEGACY)
+ if nonce != nil {
+ enc = true
+ }
+
+ }
Review comment:
both if blocks will always be run but once enc is set to true it cannot be unset
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] utzig commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
utzig commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r691480191
##########
File path: image/image.go
##########
@@ -813,3 +813,25 @@ func (img *Image) Bin() ([]byte, error) {
return b.Bytes(), nil
}
+
+// HasEncryptionPayload indicates whether an image's contains a HW encryption payload.
+func (img *Image) HasEncryptionPayload() bool {
+ enc := false
+ res, _ := img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID_LEGACY)
+ if res != nil {
+ nonce, _ := img.FindAllUniqueTlv(IMAGE_TLV_AES_NONCE_LEGACY)
+ if nonce != nil {
+ enc = true
+ }
+
+ }
Review comment:
The block below only needs to run if `!enc`, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] agross-korg commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r686293161
##########
File path: image/image.go
##########
@@ -795,13 +795,14 @@ func (img *Image) IsEncrypted() bool {
enc := false
if img.Header.Flags&IMAGE_F_ENCRYPTED != 0 {
enc = true
- } else {
- for _, tlv := range img.ProtTlvs {
- if tlv.Header.Type&IMAGE_TLV_SECRET_ID_LEGACY != 0 || tlv.Header.Type&IMAGE_TLV_SECRET_ID != 0 {
- enc = true
- }
+ } else if res, _ := img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID_LEGACY); res != nil {
Review comment:
I think all of the If / else need to be inside this else {}. In addition, I'd split the assignment and check of the res into two lines.
res, _ := img.FindAllUniqueTlv(XXXXXXX);
if (res != nil } {.....}
etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] utzig merged pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
utzig merged pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] WeekendSuperhero commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
WeekendSuperhero commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r691484760
##########
File path: image/image.go
##########
@@ -813,3 +813,25 @@ func (img *Image) Bin() ([]byte, error) {
return b.Bytes(), nil
}
+
+// HasEncryptionPayload indicates whether an image's contains a HW encryption payload.
+func (img *Image) HasEncryptionPayload() bool {
+ enc := false
+ res, _ := img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID_LEGACY)
+ if res != nil {
+ nonce, _ := img.FindAllUniqueTlv(IMAGE_TLV_AES_NONCE_LEGACY)
+ if nonce != nil {
+ enc = true
+ }
+
+ }
Review comment:
there are two separate tlv indicators that could point to a hw enc. the legacy secret id or the current secret id, we need to check for both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] utzig commented on pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
utzig commented on pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#issuecomment-896328324
@WeekendSuperhero Please squash all those commits with a useful commit message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] WeekendSuperhero commented on pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
WeekendSuperhero commented on pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#issuecomment-899762102
> The code itself looks like it's doing the right thing, but I wonder about the semantics, because having an encrypted image is different from having a payload (TLV) with data for a HW encryption engine (which also happens to have an encrypted image but not one MCUboot would be able to decrypt, for example), so someone else using this library (assuming there is anyone) would not have it's "app"/"lib" break. Have you considered creating a `HasEncryptionPayload` function (or something similar) and then from the place you need this functionality doing `IsEncrypted() || HasEncryptionPayload()`? Would that solve the problem?
I pushed up a change with the requested feature set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] utzig commented on a change in pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
utzig commented on a change in pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#discussion_r691487385
##########
File path: image/image.go
##########
@@ -813,3 +813,25 @@ func (img *Image) Bin() ([]byte, error) {
return b.Bytes(), nil
}
+
+// HasEncryptionPayload indicates whether an image's contains a HW encryption payload.
+func (img *Image) HasEncryptionPayload() bool {
+ enc := false
+ res, _ := img.FindAllUniqueTlv(IMAGE_TLV_SECRET_ID_LEGACY)
+ if res != nil {
+ nonce, _ := img.FindAllUniqueTlv(IMAGE_TLV_AES_NONCE_LEGACY)
+ if nonce != nil {
+ enc = true
+ }
+
+ }
Review comment:
Sure, but my point was that if you already know there is a legacy secret/nonce, you don't need to also look for a (non-legacy) secret/nonce, or am I missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-artifact] utzig commented on pull request #32: Support Hardware Encryption is isEncrypted function
Posted by GitBox <gi...@apache.org>.
utzig commented on pull request #32:
URL: https://github.com/apache/mynewt-artifact/pull/32#issuecomment-896762104
> > @WeekendSuperhero Please squash all those commits with a useful commit message.
>
> I can squash and merge with a meaningful message when this PR is merged.
No you can't, unless you mean "approved", but what would be the point, just do it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org