Skip to content

Block action if BIB and BCB conflict on the same target - #299

Open
william-fei wants to merge 4 commits into
mainfrom
290-drop-bib
Open

Block action if BIB and BCB conflict on the same target#299
william-fei wants to merge 4 commits into
mainfrom
290-drop-bib

Conversation

@william-fei

Copy link
Copy Markdown
Contributor

Closes #290

@william-fei
william-fei requested a review from a team as a code owner August 26, 2026 15:11
@github-project-automation github-project-automation Bot moved this to Todo in BSL Aug 26, 2026
@william-fei william-fei self-assigned this Aug 26, 2026
@william-fei william-fei added sustainment Long-term sustainment activities and technical debt removal test finding Issue found during testing after code freeze. It's not necessarily a bug from a prior release labels Aug 26, 2026
@william-fei william-fei added this to the BSL v2.0.0 milestone Aug 26, 2026
@william-fei
william-fei marked this pull request as draft August 26, 2026 15:16

@BrianSipos BrianSipos left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It could be good to capture a valid/invalid use of this in an mock BPA test, which would be a stand-alone example of what works and what does not.

From the standpoint of real use cases, I don't know that a verifier role for both ops with the same target makes sense anyway. Verifying a BCB will check the integrity of the target plaintext because all encrypt algos available to BSL are AEAD and use an auth tag for integrity.

Comment thread src/bsl/sample_pp/SamplePolicyProvider.c Outdated

@jeronstone jeronstone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#292 is now merged, which includes a MockBPA test for this. Remove the _ from _test_json_verify_bib_bcb in test_json_policy (and, you may need to change the failure condition / expected output)

@william-fei william-fei changed the title drop bib secop if bib and bcb share target during verify Block action if BIB and BCB share target during verify Sep 2, 2026
@william-fei
william-fei marked this pull request as ready for review September 2, 2026 18:34

@jeronstone jeronstone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, there needs to be a refactor of the Query function. That can be a separate ticket, though.

One idea is to add a map { target : secop (list) } to make it easier to determine conflicts/invalid ops/etc. That would prevent the n^2 search for conflicting secops

Comment thread mock-bpa-test/test_json_policy.py
Comment thread src/bsl/sample_pp/SamplePolicyProvider.c Outdated
Comment thread src/bsl/sample_pp/SamplePolicyProvider.c Outdated
Comment thread src/bsl/sample_pp/SamplePolicyProvider.c Outdated
Comment thread src/bsl/sample_pp/SamplePolicyProvider.c Outdated
@william-fei william-fei changed the title Block action if BIB and BCB share target during verify Block action if BIB and BCB conflict on the same target Sep 4, 2026
@sonarqubecloud

sonarqubecloud Bot commented Sep 4, 2026

Copy link
Copy Markdown

@william-fei
william-fei requested review from BrianSipos and jeronstone and removed request for BrianSipos September 4, 2026 21:28
@BrianSipos BrianSipos moved this from Todo to In Progress in BSL Sep 8, 2026
static bool BSLP_SecOperHasConflict(const BSL_SecOper_t *sec_oper, const BSLP_SecOperPtrList_t secops)
{
if (!BSL_SecOper_IsBIB(sec_oper)
|| (!BSL_SecOper_IsRoleVerifier(sec_oper) && !BSL_SecOper_IsRoleAcceptor(sec_oper)))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not-verifier and not-acceptor is the same as source role, correct?

const uint64_t target_block_num = BSL_SecOper_GetTargetBlockNum(sec_oper);
for (size_t i = 0; i < BSLP_SecOperPtrList_size(secops); i++)
{
const BSL_SecOper_t *comp = *BSLP_SecOperPtrList_cget(secops, i);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this logic properly handle the case when the oper instance is already in the list (by-pointer equality comp == sec_oper)?

Comment on lines +324 to +329
for (size_t j = 0; j < BSLP_SecOperPtrList_size(secops); j++)
{
BSL_SecOper_t *discarded_secop = *BSLP_SecOperPtrList_get(secops, j);
BSL_SecOper_Deinit(discarded_secop);
BSL_free(discarded_secop);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is duplicative of the BSLP_SecOperPtrList_clear below, since shared pointers are used.

Also, there is a little bit of backward logic here; it may be more readable to have a scope-local bool valid = true and then if conflict valid = false; and outside of this loop an if !valid clear/deinit/free/return.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sustainment Long-term sustainment activities and technical debt removal test finding Issue found during testing after code freeze. It's not necessarily a bug from a prior release

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

BIB attempts to verify encrypted target

3 participants