search

ppp-project / ppp

Paul's PPP Package: PPP daemon and associated utilities | Official GitHub repo: https://github.com/ppp-project/ppp
https://github.com/ppp-project/ppp
Other
384 stars 231 forks source link

Add EAP-TLS in main code #131

Closed Neustradamus closed 3 years ago

Neustradamus commented 4 years ago

Please add EAP-TLS in the main code:

jjkeijser commented 4 years ago

+1 from me (the author of the patch). Please let me know what extra work would be needed to merge the patch. Others expressed their interest in this as well, and my last patch should compile on all Linux distros without any problems. As Solaris does not seem to support MPPE I never adapted my patch for it.

Neustradamus commented 4 years ago

@jjkeijser: Can you create a PR?

jjkeijser commented 4 years ago

I will create a PR at the end of January. I am currently away from work so I don't have full access to my development environment.

paulusmack commented 4 years ago

I assume you have fixed CVE-2018-11574?

jjkeijser commented 4 years ago

Hi,

That was fixed in v1.101 of the patch. For inclusion into the main line of code I'd use a newer version, so yes it has been fixed.

JJK

On Feb 7, 2020, 23:38, at 23:38, Paul Mackerras notifications@github.com wrote:

I assume you have fixed CVE-2018-11574?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/paulusmack/ppp/issues/131#issuecomment-583646812

enaess commented 4 years ago

What's the status of this patch now? Any chance this can be committed to master branch?

jjkeijser commented 4 years ago

Hi all,

On 13/02/20 16:55, Eivind Næss wrote:

What's the status of this patch now? Any chance this can be committed to master branch?

sorry for the long delay,but I think I've just managed to create my first github pull request :)

Summary:

Added EAP-TLS support based on ppp-2.4.7-eaptls-mppe-1.200.patch with support for

This patch also addresses CVE-2018-11574 and also build on Solaris (x86 tested).

Previous patch: https://www.nikhef.nl/~janjust/ppp/ppp-2.4.7-eaptls-mppe-1.102.patch

Please let me know if you have any questions.

Share and enjoy,

JJK / Jan Just Keijser

enaess commented 4 years ago

@paulusmack - what's the status of this, could we have this merged please?

paulusmack commented 4 years ago

Some questions about this:

I asked James Carlson to look at the patch, and he sent some comments to me via email. I have pasted in his comments below. The comment about the code only being able to do EAP-TLS when this patch is included is troubling (though I haven't yet verified for myself that his comment is valid).

James Carlson's comments:

"This looks a lot better thought-through than the other patch, but it does have a few small issues. One is the way EAP_DEFTRANSMITS is redefined. That seems wrongish to me. It certainly shouldn't be a conditional compilation. If it's needed, then perhaps it should be part of the options processing instead -- i.e., if you've enabled EAP-TLS at run time, then the transmit maximum (or maybe the timeout or maybe both) is increased to accommodate the TLS goop.

Overall, a lot of the change looks like this. It's nice that it's conditionally compiled into the code. That part is good. But when the compilation is enabled, the resulting pppd should not be a special purpose EAP-TLS-only module. It should still work normally by default, and just have new options.

The difference here is in usage model. I think the author of this patch is addressing individual users who will download this patch and then use EAP-TLS only, because that's the particular need they have. But if we integrate this, then a distributor is likely to enable this feature by default when compiling a package, meaning that all the downstream users end up getting the modified features, including the ones that aren't really right for normal PPP.

Some parts look like generic new features in the code (such as tracking the previous state), and probably shouldn't be conditional on the use of TLS. They're only tangentially related and shouldn't need an ifdef for future maintainers to stumble over."

jjkeijser commented 4 years ago

In regard to your questions: from my pppd-EAP-TLS webpage https://www.nikhef.nl/~janjust/ppp/index.html :

Note that the current version of my patch falsely claims TLS1.3 support - that should be TLS 1.2, as there is no official RFC yet for EAP-TLS+TLS1.3.

How to test: I've written up a tutorial on how to set up and test pptp+ppp+EAP-TLS at https://www.nikhef.nl/~janjust/ppp/documentation.html

As for James Carlson's comments: my EAP-TLS patch does NOT interfere with the other modes of operation of pppd; you can still use it without eap-tls, I have tested that pap & ms-chap-v2 setups still work. So this is NOT an EAP-TLS-only usecase. Having said that, I'd like to point out the fact that a lot of distro's include my patch in their pppd builds by default: Fedora, RHEL, CentOS, Debian, Ubuntu, ArchLinux and perhaps others. If there was any problem with this being an EAP-TLS-only thing then I am sure I would have heard about it by now.

Regarding the comment about "EAP_DEFTRANSMITS" : yes this was increased for EAP-TLS, as a TLS certificate chain can be quite long and you might need to transmit more than 10 packets during the initial handshake. I have no problem to raise the default to 30 (the TLS default) for all builds, but that went against the idea behind my patch.

Finally, I'd like to point out the reason for this PR: my EAP-TLS patch has existed for over 14 years and has been included in mainstream Linux distro's for over 10 years. I've been asked several times why my patch was never included in the mainstream code. Therefore I have put together this PR, as I do not want to continue to keep this patch on my homepage fowever. However, if this PR is rejected then I will simply continue to provide the patch there. I have no desire (nor the time!) to do major rewrites.

Neustradamus commented 4 years ago

@jjkeijser: I have seen in this document: https://w1.fi/cgit/hostap/plain/hostapd/ChangeLog

Linked to?

jjkeijser commented 4 years ago

I've just posted two new versions of my ppp-eap-tls patch:

enaess commented 4 years ago

Can we get this patch merged into the project?

Neustradamus commented 4 years ago

@paulusmack: Any news? A merging and a new release soon?

Several users and developers wait you...

Sander80 commented 4 years ago

Greetings. I see that currently the repository of jjeikser is missing. Is this intentional?

Anyway that code exists currently in my repo https://github.com/Sander80/ppp I've been making pull requests to the repostory of jjeikser..

Sander80 commented 4 years ago

And it's back... jjeikser, what is the best way to contact you? We've got some patches for your eap-tls patch and a way to test it: https://github.com/Sander80/l2tp-ipsec-vpn

jjkeijser commented 4 years ago

Hi,

On 27/05/20 13:01, Sander80 wrote:

And it's back... jjeikser, what is the best way to contact you? We've got some patches for your eap-tls patch and a way to test it: https://github.com/Sander80/l2tp-ipsec-vpn

I was having problems with my fork of the ppp code and was forced to recreate it from scratch. I merged in the fix to provide a method to pass password/PIN to the PKCS#11 backend, based on the patches you and Petr submitted. Also, in my latest PR I tried to minimize the changes, stripping out the feature to use MD5&SHA1  from OpenSSL. This was a requirement earlier, to allow ppp+EAP-TLS to build against OpenSSL 1.0.0. I am dropping support for that also, requiring a version of OpenSSL that has the "SSL_export_keying_material()" call.

Adding the option to switch between ppp-supplied md4/md5/sha1 calls versus the ones provided by OpenSSL is for a future patch.

cheers,

JJK

Sander80 commented 4 years ago

Please have a look at this fix https://github.com/Sander80/ppp/commit/cd41cfe20da8e415e6eebff788479de377f718f0

I was trying to make a pull request to you with this, before your repo was gone. Your implementation of our fix for pins did not work, because UI_METHOD* transfer_pin = UI_create_method("transfer_pin"); is defined in a block and can be compiler-optimized outside.

Sander80 commented 4 years ago

I made out a new pull request for you to the current code state

jjkeijser commented 4 years ago

On 27/05/20 13:13, Sander80 wrote:

Please have a look at this fix Sander80@cd41cfe https://github.com/Sander80/ppp/commit/cd41cfe20da8e415e6eebff788479de377f718f0

I was trying to make a pull request to you with this, before your repo was gone. Your implementation of our fix for pins did not work, because UI_METHOD* transfer_pin = UI_create_method("transfer_pin"); is defined in a block and can be compiler-optimized outside.

I don't understand why your patch is much different; the significant difference consists of two dbglog statements - UI_create_method() is still defined inside a block...  can you provide me with more info why my implementation does not work for you?

Sander80 commented 4 years ago

Let us move to your repository with the disscussion not to spam here ) https://github.com/jjkeijser/ppp/pull/1

Neustradamus commented 3 years ago

Victory, it has been merged: https://github.com/paulusmack/ppp/pull/172

@jjkeijser: Thanks a lot for your big contribution in the main PPP code (originally a patch: https://www.nikhef.nl/~janjust/ppp/) @Sander80, @lo1ol, @enaess: Thanks for your comments @paulusmack: Thanks for your comments, and merging

The old PR: https://github.com/paulusmack/ppp/pull/151

enaess commented 3 years ago

Wohaa!! Great work everyone 👍🏼

Neustradamus commented 3 years ago

@yarda: About your comment, it is merged, it is in master.

@jjkeijser: Maybe you can inform people on your website that it has been merged and specify the date...

jjkeijser commented 3 years ago

@Neustradamus: thanks for the tip, done.

On 29/12/20 06:42, Neustradamus wrote:

@yarda https://github.com/yarda: About your comment, it is merged, it is in master.

@jjkeijser https://github.com/jjkeijser: Maybe you can inform people on your website that it has been merged and specify the date...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/paulusmack/ppp/issues/131#issuecomment-751954443, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWR3ZO6ZDFP5WJP5SGNBNLSXFT6HANCNFSM4KBQXYCA.

Neustradamus commented 3 years ago

@jjkeijser : Thanks for the added note :)

Note: Already one year that I have done this ticket, happy birthday: