-
Notifications
You must be signed in to change notification settings - Fork 118
Frame Encryption for Cloud Recording #545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for restoring the great approach. Two detail questions:
|
|
We asked ourselves the same when reviewing for the KID count, we're not sure we restricted it like that originally. One guess was that it would help keep the size down since this box needs to be sent to the key server. We've validated that it is compliant per the spec, but we don't feel super strongly about it either way. Good point for the thumbprint length, I need to push an update to add the encryption version like we discussed in Istanbul and I'll make the thumbprint fully adjustable. My thinking is that whatever thumbprint algorithm would then be dictated by the encryption version in the box combined with the security baseline document. I'll try to do that by tomorrow |
|
Please double check the early draft SecurityBaseline and let me know in case something is missing. |
doc/RecordingControl.xml
Outdated
| <para> | ||
| When using <literal>Key/KID</literal> then the device shall interpret the configured <literal>KID</literal> for the encryption entry as a 16-byte hexadecimal value and use this value as the key identifier. | ||
| The device shall include a Standard CENC PSSH box according to <xref linkend="_refKeyStorageStandardCenc" />. | ||
| </para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest moving to section Standart ENC System
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformulated the whole section, as the W3C CENC pssh should be present all the time, not just for Key/KID... It should also read easier now.
doc/RecordingControl.xml
Outdated
| The device shall include a Standard CENC PSSH box according to <xref linkend="_refKeyStorageStandardCenc" />. | ||
| </para> | ||
| <para> | ||
| When using <literal>AsymmetricEncryption</literal> then the device shall generate a <literal>KID/Key</literal> pair and encrypt the <literal>Key</literal> with each certificate defined in the configuration. The encrypted keys shall be stored in the file using the Asymmetric key system PSSH box as defined in <xref linkend="_refKeyStorageKeyRotationSystem" />. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to move this to section 8.1.2 Asymmetric key system.
doc/RecordingControl.xml
Outdated
| <section xml:id="_refKeyStorageStandardCenc"> | ||
| <title>Common Encryption standard system</title> | ||
| <para> | ||
| When a file is frame encrypted using a <literal>Key/KID</literal> then exactly one pssh box of type 'CENC' box shall be present in the file according to [W3C 'cenc' Initialization Data Format]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the condition 'When a file is frame encrypted' mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworded the entire section. Should be better now
doc/RecordingControl.xml
Outdated
| <para><literal>KID</literal> is the generated UUID by the device. This <literal>KID</literal> should be the same in all segments until the symmetric key changes.</para> | ||
| <para><literal>DataSize</literal> is the size in bytes of all the other fields present in this box following this field.</para> | ||
| <para><literal>EncryptedKeyEntryCount</literal> Number of entries containing encryption information required to decrypt the symmetric key. Shall be 1 or more.</para> | ||
| <para><literal>CertificateThumbprintAlgorithm</literal> defines the algorithm used to compute the thumbprint of the certificates. Those values are defined in the Security Baseline specification</para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security baseline does not define identifiers. Suggest to refer to RFC 6920 and IANA Named Information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure those are the right ones though... Certificate thumbprints seems to generally be defined through OIDs... or maybe their friendly names? Like "sha256WithRSAEncryption"?
But now that I'm discussing it with colleagues, I'm wondering if that information is actually useful at all? If we have the CertificateThumbprint itself, the Client has configured the certificate, and doesn't really need to know how the thumbprint was computed, just what it is so it can find it again in a certificate list. Unless we're missing something, maybe this field is useless and we just need the ThumbprintSize + the thumbprint itself?
|
@HansBusch just to notify you of some cleanups in the past few days. We've changed the SystemId guid due to Axis having shipped the previous schema already, I also moved the EncryptedKeySize around to match both versions and they now have the same size (one was 16bits and the other 32...) |
|
As the client cannot set any HPKE parameters it is up to the device to choose some. Is the expectation that a client can decrypt every combination and the device is free to pick a combination? |
| <xs:annotation> | ||
| <xs:documentation> | ||
| Frequency at which the device shall generate a new key to encrypt a new segment. | ||
| If not specified, key rotation is disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default disabling key rotation is not a good security design. I suggest to rephrase to If not specified the behavior is device implementation dependent..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll raise the point during F2F. I believe we should make this mandatory instead of the optional.
If a user wants fixed keys, they can use the old mechanism of KID+Key field anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Reviewed all files and comments and also reviewed all text and images via the developer portal.
I will be reviewing separately the #625.
patrickrad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
As discussed during the Instanbul F2F, this restores the previous pull request #315.
This adds support for