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

[ARC-144] add if condition for HTTP target, allow winrm connection #178

Conversation

irvingoujAtDevolution
Copy link
Contributor

winrm connection seems disagree with Ker5 user to user protocol, add a new if condition, if the target is http (aka winrm server), then use plain kerbero5 protocol instead of user to user

feat: add if condition for HTTP target
@irvingoujAtDevolution irvingoujAtDevolution force-pushed the ARC-144-sspi-rs-WASM-module-and-JavaScript-API-for-NTLM branch from 1fd8f76 to bef493f Compare October 6, 2023 18:14
src/kerberos/mod.rs Outdated Show resolved Hide resolved
src/kerberos/mod.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Thank you!

Cargo.toml Outdated Show resolved Hide resolved
examples/kerberos.rs Outdated Show resolved Hide resolved
examples/kerberos.rs Outdated Show resolved Hide resolved

fn main() -> Result<(), Box<dyn Error + Send + Sync>> {
tracing_subscriber::fmt()
.with_max_level(tracing::Level::DEBUG) // Adjust level as needed
Copy link
Member

Choose a reason for hiding this comment

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

Use EnvFilter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

udpated

Copy link
Member

Choose a reason for hiding this comment

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

Marking as unresolved as you do not appear to be using EnvFilter yet

examples/kerberos.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated
rustls = { version = "0.21", features = ["dangerous_configuration"], optional = true }
rustls = { version = "0.21", features = [
"dangerous_configuration",
], optional = true }
Copy link
Member

@CBenoit CBenoit Oct 6, 2023

Choose a reason for hiding this comment

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

Please, do you think you could configure your editor to not automatically re-format all Cargo.toml you touch? It would be preferable so we don’t end up with noise, and don’t deviate too much from other Rust projects.

Most Rust projects typically does not use this formatting. This is well explained by the style guide from the Rust compiler: "[f]or array values, such as a list of features, put the entire list on the same line as the key, if it fits".

I would prefer we wait for official support in rustfmt before we apply such automatic formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, both the my disabled my vscode formatter and the file. Do you have a recommanded toml formatter tool for this?

Copy link
Member

Choose a reason for hiding this comment

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

Sadly there is currently no blessed formatter for Cargo.toml as of today (but that would be rust-lang/rustfmt#5240 when it’s ready)

@@ -889,7 +1075,7 @@ impl<'a> Kerberos {
let encoded_auth = picky_asn1_der::to_vec(&authenticator)?;
info!(encoded_ap_req_authenticator = ?encoded_auth);

// FIXME: properly negotiate mech id - Windows always does KRB5 U2U
// FIXME: properly negotiate mech id - Windows always does KRB5 U2U for Non-HTTP Target
Copy link
Member

Choose a reason for hiding this comment

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

Is it an issue we should track seriously, and create an issue for it?
cc @awakecoding

Comment on lines 725 to 726
// 4 = size of u32
nonce: &OsRng.gen::<[u8; 4]>(),
Copy link
Member

Choose a reason for hiding this comment

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

Note that you could use core::mem::size_of::<u32>() so the code is self-explanatory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is direct copy from original code

Copy link
Member

@CBenoit CBenoit Oct 7, 2023

Choose a reason for hiding this comment

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

Oh okay, I understand. Do you think you could address this anyway while you’re at it? Thank you

Note: another alternative is

OsRng.gen::<u32>().to_ne_bytes(),

src/kerberos/mod.rs Outdated Show resolved Hide resolved
src/kerberos/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
username,
password: password.into(),
domain: None,
#[cfg(feature = "network_client")]
Copy link
Member

@CBenoit CBenoit Oct 7, 2023

Choose a reason for hiding this comment

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

You can put this at the top of the file:

#![cfg(feature = "network_client")]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is that cargo test needs to see the main function present in the file. if I put it on top, it will not pass cargo test

Copy link
Member

Choose a reason for hiding this comment

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

You’re right! Actually, in this case you should remove the#[cfg]s altogether, and add this at the bottom of the Cargo.toml instead:

[[example]]
name = "kerberos"
required-features = ["network_client"]

This specifies that this example needs the "network_client" feature enabled in order to be compiled and it will be ignored if the feature is not available.

@irvingoujAtDevolution
Copy link
Contributor Author

I will keep this PR as draft, just in case I found things not working with my winrm client

@irvingoujAtDevolution
Copy link
Contributor Author

Not correct path, problem solved without using the newly implemented variation of Kerb5 protocol

@irvingoujAtDevolution irvingoujAtDevolution deleted the ARC-144-sspi-rs-WASM-module-and-JavaScript-API-for-NTLM branch October 18, 2023 13:46
@irvingoujAtDevolution irvingoujAtDevolution restored the ARC-144-sspi-rs-WASM-module-and-JavaScript-API-for-NTLM branch October 27, 2023 18:25
@CBenoit CBenoit deleted the ARC-144-sspi-rs-WASM-module-and-JavaScript-API-for-NTLM branch March 7, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants