Skip to content
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

20231101 kanidm integration #18

Merged
merged 8 commits into from
Nov 3, 2023

Conversation

Firstyear
Copy link
Member

WIP for the changes needed to integrate this to Kanidm's Unix Integration.

I'm not 100% sure about this. It feels "messy". I was trying to use the HSM trait for dynamic dispatch so that the Kani unix resolver didn't have to care about the type of hsm that was in use. But the issue there is we then need Box<dyn Hsm<MachineKey = Type, LoadableMachineKey = Type, ....>>. Effectively we end up in a really awkward spot if we use associated types.

This changes to use enums for runtime dynamic dispatch - currently I half-baked it, wrapping the types, but I feel we just probably need to flatten to one giant "MachineKey" enum and each HSM type pattern matches over it. Currently it's

#[derive(Debug, Clone, Serialize, Deserialize)]
pub enum LoadableMachineKey {
    Soft(soft::SoftLoadableMachineKey),
    #[cfg(feature = "tpm")]
    Tpm(tpm::TpmLoadableMachineKey),
    #[cfg(not(feature = "tpm"))]
    Tpm(()),
}

Flattened would be

#[derive(Debug, Clone, Serialize, Deserialize)]
pub enum LoadableMachineKey {
    SoftAes256GcmV1 {
        key: Vec<u8>,
        tag: [u8; 16],
        iv: [u8; 16],
    },
    #[cfg(feature = "tpm")]
    TpmAes128CfbV1 {
        private: TpmPrivate,
        public: TpmPublic,
    },
    #[cfg(not(feature = "tpm"))]
    TpmAes128CfbV1 {
        invalid: ()
    }
}

Maybe the design here is flawed? Maybe there is a better structure? The goal was that the resolvers would just get a "dyn Hsm" and be able to interact with it, so they don't need to care where their keys are, and they trust we did the work for them.

I think to do this, maybe we just need to accept the enum's? Traits are making this too hard.

cc @micolous for some advice.

Checklist

  • [ x ] This pr contains no AI generated code
  • cargo fmt has been run
  • cargo clippy has been run and there's no issues
  • cargo test has been run and passes

Copy link
Collaborator

@dmulder dmulder left a comment

Choose a reason for hiding this comment

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

Like you said, I think you're maybe overthinking it a bit. The end result is either way, these implementation details are hidden from the end user. I think the current approach is fine.

@Firstyear
Copy link
Member Author

Yeah. I kept going with this approach and tidied it up and I think I'm happy with it :) Thanks for the double check. I'll update this PR once I improve it more.

@Firstyear Firstyear marked this pull request as ready for review November 3, 2023 05:10
@Firstyear
Copy link
Member Author

@dmulder This is good to go now. I renamed it from hsm to tpm mainly because some of these functions are tpm specific and hsm/pkcs11 is a little different.

Copy link
Collaborator

@dmulder dmulder left a comment

Choose a reason for hiding this comment

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

LGTM

@Firstyear Firstyear merged commit cecc21f into kanidm:main Nov 3, 2023
8 checks passed
@Firstyear Firstyear deleted the 20231101-kanidm-integration branch November 3, 2023 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants