Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

better remove #[derive(Default)] in bitflags data structure #64

Open
longlongyang opened this issue Aug 30, 2023 · 2 comments
Open

better remove #[derive(Default)] in bitflags data structure #64

longlongyang opened this issue Aug 30, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@longlongyang
Copy link
Contributor

When debugging the issue #63 , I found that, the match pattern doesn't check the default value 0, the value 0 means responder doesn't support mutual authentication feature.
But when creating object and setting bits on the bitflags object, default value 0 is used, it may result in inconsistency.

I would recommend to remove all #[derive(Default)] in bitflags data structure, and add default item to the bitflags data structure explicitly, then It may minimize the gap programmer make mistakes.

@longlongyang
Copy link
Contributor Author

Example:
consider

bitflags! {
    #[derive(Default)]
    pub struct SpdmKeyExchangeMutAuthAttributes: u8 {
        const MUT_AUTH_REQ = 0b00000001;
        const MUT_AUTH_REQ_WITH_ENCAP_REQUEST = 0b00000010;
        const MUT_AUTH_REQ_WITH_GET_DIGESTS = 0b00000100;
    }
}

the match pattern

match mut_auth_requested {
            SpdmKeyExchangeMutAuthAttributes::MUT_AUTH_REQ => Ok(()),
            SpdmKeyExchangeMutAuthAttributes::MUT_AUTH_REQ_WITH_ENCAP_REQUEST
            | SpdmKeyExchangeMutAuthAttributes::MUT_AUTH_REQ_WITH_GET_DIGESTS => {
                self.get_encapsulated_request_response(session_id, mut_auth_requested)
                    .await
            }
            _ => Err(SPDM_STATUS_INVALID_MSG_FIELD),
        }

in this case, when responder doesn't support mutual authentication, the value of mut_auth_requested of requester will be SpdmKeyExchangeMutAuthAttributes::empty() which is 0,
By pattern match, requester will run into Err(SPDM_STATUS_INVALID_MSG_FIELD). This is not a correct behavior, requester should just ignore the mutual authentication and proceed instead of running into error and break the start session.

My suggestion,

change definition of SpdmKeyExchangeMutAuthAttributes to:

bitflags! {
    pub struct SpdmKeyExchangeMutAuthAttributes: u8 {
        const MUT_AUTH_DISABLED = 0b00000000;
        const MUT_AUTH_REQ = 0b00000001;
        const MUT_AUTH_REQ_WITH_ENCAP_REQUEST = 0b00000010;
        const MUT_AUTH_REQ_WITH_GET_DIGESTS = 0b00000100;
    }
}

impl Default for SpdmKeyExchangeMutAuthAttributes {
    fn default(&self) -> Self {
        self::MUT_AUTH_DISABLED
    }
}

the match pattern

match mut_auth_requested {
            SpdmKeyExchangeMutAuthAttributes::MUT_AUTH_REQ => Ok(()),
            SpdmKeyExchangeMutAuthAttributes::MUT_AUTH_REQ_WITH_ENCAP_REQUEST
            | SpdmKeyExchangeMutAuthAttributes::MUT_AUTH_REQ_WITH_GET_DIGESTS => {
                self.get_encapsulated_request_response(session_id, mut_auth_requested)
                    .await
            },
           SpdmKeyExchangeMutAuthAttributes::MUT_AUTH_DISABLED => Ok(()),
        }

the benefit is that, there are only 4 explicit kinds of value in matching SpdmKeyExchangeMutAuthAttributes, compiler will force programmer to implement logic for all of them.

@jyao1
Copy link
Contributor

jyao1 commented Aug 30, 2023

looks good to me

@xiaoyuxlu xiaoyuxlu added the enhancement New feature or request label Sep 15, 2023
longlongyang added a commit to longlongyang/rust-spdm that referenced this issue Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants