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

Add ability to load ccache from asrep and aes key #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmulder
Copy link

@dmulder dmulder commented Nov 6, 2024

No description provided.

Ok(DerivedKey::Aes256CtsHmacSha196 {
k,
i: 0,
s: "".to_string(),
Copy link
Author

Choose a reason for hiding this comment

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

@Firstyear I'd like your opinion here, since I'm not sure the appropriate way to handle this. See the way libhimmelblau currently decrypts the enc_part here: https://gitlab.com/samba-team/libhimmelblau/-/blob/main/src/auth.rs?ref_type=heads#L808

Copy link
Author

Choose a reason for hiding this comment

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

So there are a few problems. MS still supports Aes128 (libkrimes does not). And then I wasn't sure what to do with these iteration and salt values. FYI, this client_key is encrypted using the session_key, and therefore TPM bound.

Copy link
Member

Choose a reason for hiding this comment

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

The question is what the client key represents - realistically what you have is more likely the SessionKey ( https://github.com/kanidm/libkrimes/blob/main/src/proto/mod.rs#L276 ) since the DerivedKey generally comes from a password.

Within the operation of KRB, it's the SessionKey that's used in TGS REQ ( https://github.com/kanidm/libkrimes/blob/main/src/proto/request.rs#L232 ) and the ccache is storing the same https://github.com/kanidm/libkrimes/blob/main/src/ccache.rs#L217

So I suspect you actually need to drop this fn and use the SessionKey instead here :)

The DerivedKey is only used during AS-REQ/REP, and I think from what you have indicated, you aren't doing an AS-REQ, but you just get an encrypted AS-REP that is bound to the TPM (Rather than the DerivedKey of the user).

Re the AES128, we can add that later, it shouldn't be too hard after Aes256 at this point.

Copy link
Author

Choose a reason for hiding this comment

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

It's not the session key. The client key is encrypted using the session key, and is included with the as-rep in the json response from Azure.
The source I referenced which I'm currently using is actually functional as is (I can use that client key to decrypt the enc_part of the asrep now). From what I can tell, it's used in exactly the same way as the DerivedKey, but there is no password, no salt, no iterations. I realize it 'generally comes from a password'. I'm saying in this case it does not. MS has done something weird here.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, we're talking about 2 different 'session keys'. Sorry, I don't really know.

if !val.starts_with("FILE:") {
return Err(KrbError::UnsupportedCredentialCacheType);
}
let p = val.strip_prefix("FILE:").expect("Failed to strip prefix");
Copy link
Author

Choose a reason for hiding this comment

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

FYI, I'm thinking we should support DIR here, so we can stash the ccache in a file named username@realm, for example, within the DIR sub-directory.

Copy link
Author

Choose a reason for hiding this comment

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

I also think we should be able to specify the DIR from the function call. We could have the path parameter be a enum:

enum CCachePath {
    FILE(String),
    DIR(String),
}

etc

Copy link
Member

Choose a reason for hiding this comment

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

Probably better that we pursue the kernel keyring rather than directory here I think. The file mode is a good easy test, but keyring is likely more secure.

Copy link
Author

Choose a reason for hiding this comment

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

It also occurs to me that this is better supported outside this library. Recall that the idprovider will be fetching the AsRep, but it's the task_daemon that will need to install the ccache somewhere, IIUC. So perhaps this library should just respond with a ccache as a Vec<u8>?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's not a good option - it would be better to respond with the ccache as a strong type because then the caller can read the file, get the cache, and then if they get session keys they can add them, remove them, manage them, inspect them etc.

If we made this vec<u8> we lose all that.

Ok(DerivedKey::Aes256CtsHmacSha196 {
k,
i: 0,
s: "".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

The question is what the client key represents - realistically what you have is more likely the SessionKey ( https://github.com/kanidm/libkrimes/blob/main/src/proto/mod.rs#L276 ) since the DerivedKey generally comes from a password.

Within the operation of KRB, it's the SessionKey that's used in TGS REQ ( https://github.com/kanidm/libkrimes/blob/main/src/proto/request.rs#L232 ) and the ccache is storing the same https://github.com/kanidm/libkrimes/blob/main/src/ccache.rs#L217

So I suspect you actually need to drop this fn and use the SessionKey instead here :)

The DerivedKey is only used during AS-REQ/REP, and I think from what you have indicated, you aren't doing an AS-REQ, but you just get an encrypted AS-REP that is bound to the TPM (Rather than the DerivedKey of the user).

Re the AES128, we can add that later, it shouldn't be too hard after Aes256 at this point.

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.

2 participants