Fix default ciphers in the configurations #274
Conversation
Signed-off-by: Marc Emmers <[email protected]>
| @@ -222,7 +222,7 @@ | |||
| "mutability": "ReadOnly" | |||
| } | |||
| ], | |||
| "default": "TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256", | |||
| "default": "TLS_AES_256_GCM_SHA384:TLS_AES_128_GCM_SHA256", | |||
There was a problem hiding this comment.
We should also add the TLS_CHACHA20_POLY1305_SHA256 here. Because it should be implemented according to TLS 1.3 RFC.
There was a problem hiding this comment.
cc: @hikinggrass (as you've added this file)
There was a problem hiding this comment.
I think we only added what the spec mentions what shall be supported here (A00.FR.318 says that a CSMS SHALL support at least):
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_RSA_WITH_AES_128_GCM_SHA256
TLS_RSA_WITH_AES_256_GCM_SHA384
And A00.FR.319 mentions that a charging station shall support at least either
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
and
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
or
TLS_RSA_WITH_AES_128_GCM_SHA256
and
TLS_RSA_WITH_AES_256_GCM_SHA384)
But we are free to add better ciphers here if available 😄
There was a problem hiding this comment.
I would suggest keeping the default to the bare minimum required from OCPP perspective to provide the best interoperability on all charging stations. Maybe it is good to suggest in a adoption guide or something that users could add TLS_CHACHA20_POLY1305_SHA256 if they want.
| "SupportedCiphers12": { | ||
| "variable_name": "SupportedCiphers12", | ||
| "attributes": { | ||
| "Actual": "ECDHE-ECDSA-AES128-GCM-SHA256,ECDHE-ECDSA-AES256,GCM-SHA384,AES128-GCM-SHA256,AES256-GCM-SHA384,TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256" | ||
| "Actual": "ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:AES128-GCM-SHA256:AES256-GCM-SHA384" | ||
| } | ||
| }, | ||
| "SupportedCiphers13": { | ||
| "variable_name": "SupportedCiphers13", | ||
| "attributes": { | ||
| "Actual": "TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256" | ||
| "Actual": "TLS_AES_256_GCM_SHA384:TLS_AES_128_GCM_SHA256" | ||
| } |
There was a problem hiding this comment.
I suggest we remove these configs here. There should be only one single source of truth, also for config-defaults.
If you really need to change this, then you should definitely know and understand why you're doing this anyway. Then you'll also figure out how to set this config.
There was a problem hiding this comment.
Sure but that seems out of scope for this fix, so I would prefer to leave this change in this PR.
|
Regarding the ordering of the cipher-suites, I suggest to follow Go(lang)'s standards. Especially this hint might be useful for lower end hardware:
|
In line with my earlier comment on keeping the ciphers to only the ones in the OCPP spec, I do agree that it might be better to order them as suggested there. The only change then would be moving AES128 before the AES256 counterparts. ECDHE is already preferred over RSA in these configs. |
No description provided.