Fix the master key check in osdp_cp_setup() #56
Conversation
Change the check related to the master key so that the memcpy and warning message depend only on whether or not the master key is defined.
| if (scbk_count != num_pd) { | ||
| LOG_WRN("Master key / SCBK not passed; SC Disabled."); | ||
| SET_FLAG(ctx, FLAG_SC_DISABLED); | ||
| } |
There was a problem hiding this comment.
Looks like this also fixes a bug. If master_key == NULL and scbk_count == num_pd, we copy the master_key anyway. So can you reword your commit message to have something like "Fix: you reason" (right now it feel too generic) so it will make it into the right section in the next release notes?
There was a problem hiding this comment.
AND.. In the case where we do take the master key we need to do 2 things:
- Check for
OSDP_FLAG_ENFORCE_SECUREand not do it. - Loop over all PDs and clear the
PD_FLAG_HAS_SCBKflag as src/osdp_sc.c line 42 does a check on this flag before computing the SCBK with master key.
There was a problem hiding this comment.
Just updated the commit message. You can, of course, feel to change it you can see a way to improve it.
There was a problem hiding this comment.
I was thinking the case of taking the master key was okay regarding the flags and security and an additional code change wasn't needed relating to these, at least at this time. I'll think some more though and maybe create an issue for discussion on a related topic.
|
This PR can me merged with the commit message rewording, with or without the code change suggested. Thanks for the PR. |
The logic around master_key/scbk/enforce_secure had gotten very messy. Re-arrange code to make it look slightly better. Related-to: #56 Signed-off-by: Siddharth Chandrasekaran <sidcha.dev@gmail.com>
Change the check related to the master key so that the master key is
used and accessed if and only if it is not NULL. Previously a NULL or
undefined master key could be used if the count of individually set
SCBKs was less than the number of PD.