Implement support for native EAP-TLS

[MartijnSnijder]

No description provided.

[ld0614]

Moving relevant conversation from #12

Thanks for proving that its actually much simpler than I had initially anticipated! I am very open to PRs and would love to see more people from the community get involved in the project. From only a 5 minute review on GitHub there are a couple of things I'd note:

• I would either make EapMode an Enum (if we could reasonably expect a 3rd option) or a boolean (if only supporting 13 and 25. I'm pretty split but would ur only slightly on the bool side for simplicity and this could be retrofitted later as long as the registry settings for these 2 options don't change
• I'd generally put the new option under Advanced as we still want to encourage people to use PEAP where possible
  I'd need to doublecheck nothing has gone missing from the re-factor as the diff on GitHub is a bit messed up
• Have you validated that there aren't any changes needed in WMIProfile.cs, CSPProfile.cs or VPNProfile.cs? These are important as this is what tracks if the planned profile matches the installed profile
• We'll need a couple of tests adding into the test suites to ensure that everything is working as expected and we maintain support moving forward
• The full test suite will need to be run to validate that this hasn't impacted any other known areas
• It might be worth re-baselining off of the development branch rather than main, they should be the same at the moment but could change...

I don't see anything there that I'd outright reject so please do raise a PR if your interested in continuing to get the feature into DPC and we can continue the conversation over there 😄

[ld0614]

• I'm a little confused by the parameter includeInnerServerValidation, it looks like the function is hardcoded to always be True, was there a reason for introducing this?

[ld0614]

Good News! I've done a provisional run of the test suite and its all passing so we shouldn't see any backwards compatibility issues. It would be good to get some tests into UserProfileTests.cs for a couple of major use cases and then we can thinking about merging.

We should probably add a warning and possibly reference link to advise people that PEAP + EAP TLS is better but I'm happy to update the wording of that after merge if thats easier

[MartijnSnijder]

• I'm a little confused by the parameter includeInnerServerValidation, it looks like the function is hardcoded to always be True, was there a reason for introducing this?

Good spot. That's leftover config from some logic I tested earlier, but can be removed. I'm sorry, it was still in a Proof of Concept phase when I uploaded it.

[MartijnSnijder]

One other thing I changed: I re-enabled the requirement for an issuing CA and NPS server for EAP-TLS.

Previously this requirement was relaxed for native EAP-TLS since the schema doesn’t strictly require them, which felt inconsistent and didn’t match the ADMX/ADML. I turned the requirement back on because I think that’s best practice and the safest default - Do you agree?

[ld0614]

No need to apologise! I'm just happy that I'm not having to code it 😄 Please don't take my comments negatively, I'm super excited to have people contributing to the code base and so I'm trying to match your energy when it comes to the development side and provide as much feedback as possible

[MartijnSnijder]

That’s okay 😄 I think we’re getting pretty close now. If you spot any other errors, confusing code, or inconsistencies, let me know!

[MartijnSnijder]

@ld0614 I added some tests into UserProfileTests.cs. They seem to be okay. Can you check?

[ld0614]

Yep looks like their working :)

[ld0614]

I think on the surface its all there, I do just want to rerun through all the changes and run the build personally before I commit it. I'm not entirely sure of the time frames but I'll aim to get it reviewed and assuming I don't find any concerns merged

Thanks again for the fast turnaround on this 😄

[MartijnSnijder]

Sounds good. Thanks for the quick reviews and tests. I’ll test the build myself too 🙂