Review Request: Improve handling of 802.1x certificates

Discussion:

Ilia Kats

2011-05-02 14:30:47 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------

Review request for Network Management.

Summary
-------

Currently, KDE NM is not compatible with PEM certificates (base64-encoded), which are the most used format. Attached patch makes some improvements in certificate handling, namely:
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.

This is a major change in behavior, so I would like a public review.

This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673

Diffs
-----

libs/internals/connectionpersistence.cpp 1a22c5b
libs/internals/setting.h b43365b
libs/internals/setting.cpp a637855
libs/internals/settings/802-1x.h 971ef41
libs/internals/connection.cpp 701a9e2
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc

Diff: http://git.reviewboard.kde.org/r/101275/diff

Testing
-------

Yes, see the bugzilla ticket.

Thanks,

Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110502/91b95b41/attachment.htm

Lamarque Vieira Souza

2011-05-04 20:14:42 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/#review3111
-----------------------------------------------------------

libs/internals/connection.h
<http://git.reviewboard.kde.org/r/101275/#comment2636>

Please rename this method to saveCertificates and the one below to deleteCertificates to make clear what they are saving and deleting.

libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2644>

addToCertToDelete

libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2643>

removeFromCertToDelete

libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2638>

I thinks certPathAsByteArray sounds better, just my oppinion.

libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2639>

use QString() instead of "".

libs/internals/settings/802-1x.h
<http://git.reviewboard.kde.org/r/101275/#comment2640>

Why the parameter scope is int instead of enumerate?

libs/internals/settings/802-1x.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2646>

stick to the code sytle for if { \n } else { \n }

libs/internals/settings/802-1x.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2647>

code style: move the { below to this line.

libs/internals/settings/802-1x.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2648>

code style for if () { in this entire method.

libs/ui/security/peapwidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2649>

code style for if () { \n } else { \n }

libs/ui/security/peapwidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2651>

code style for if.

libs/ui/security/peapwidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2650>

code style for if.

libs/ui/security/tlswidget.h
<http://git.reviewboard.kde.org/r/101275/#comment2652>

wrong indentation in this line and below.

libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2645>

Stick to the code style: if { \n } else { \n }

libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2653>

code style for if

libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2656>

code style for if { \n } else { \n }

libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2641>

I use QLatin1String for string constants.

libs/ui/security/tlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2654>

code style for if.

libs/ui/security/ttlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2642>

Stick to the code style and use: if () { \n
} else { \n
}

libs/ui/security/ttlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2655>

code style for if

libs/ui/security/ttlswidget.cpp
<http://git.reviewboard.kde.org/r/101275/#comment2657>

code stytle for if

- Lamarque Vieira

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 2, 2011, 2:30 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
libs/internals/connectionpersistence.cpp 1a22c5b
libs/internals/setting.h b43365b
libs/internals/setting.cpp a637855
libs/internals/settings/802-1x.h 971ef41
libs/internals/connection.cpp 701a9e2
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/7cb5aca9/attachment-0001.htm

Ilia Kats

2011-05-04 21:20:46 UTC

Permalink

Post by Ilia Kats

libs/internals/settings/802-1x.h, line 704
<http://git.reviewboard.kde.org/r/101275/diff/1/?file=15873#file15873line704>
Why the parameter scope is int instead of enumerate?

This is a virtual function, the prototype for which is declared in libs/internals/setting.h. Knm::Connection::Scope is not available there yet, as setting.h is included at the very top of libs/internals/connection.h.

- Ilia

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/#review3111
-----------------------------------------------------------

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 9:20 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/920fd1f7/attachment.htm

Ilia Kats

2011-05-04 21:20:36 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------

(Updated May 4, 2011, 9:20 p.m.)

Review request for Network Management.

Changes
-------

code cleanup

Summary
-------

Currently, KDE NM is not compatible with PEM certificates (base64-encoded), which are the most used format. Attached patch makes some improvements in certificate handling, namely:
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.

This is a major change in behavior, so I would like a public review.

This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673

Diffs (updated)
-----

CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc

Diff: http://git.reviewboard.kde.org/r/101275/diff

Testing
-------

Yes, see the bugzilla ticket.

Thanks,

Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/7dbe6f22/attachment.htm

Lamarque Vieira Souza

2011-05-04 22:35:00 UTC

Permalink

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 9:20 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/33046a96/attachment-0001.htm

Ilia Kats

2011-05-04 22:39:30 UTC

Permalink

Post by Ilia Kats

libs/internals/setting.h, line 47
<http://git.reviewboard.kde.org/r/101275/diff/2/?file=16080#file16080line47>
I think you should change the int type here to Knm::Connection::Scope.

That's just it, Knm::Connection::Scope is not yet declared here, so it does not compile. Including connection.h in setting.h yields no result either, as setting.h is included in connection.h, so that would make an infinite loop or something like that, anyway, this doesn't compile either.

- Ilia

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/#review3113
-----------------------------------------------------------

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 10:36 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/4bd1229a/attachment.htm

Lamarque Vieira Souza

2011-05-04 23:11:45 UTC

Permalink

Post by Ilia Kats

libs/internals/setting.h, line 47
<http://git.reviewboard.kde.org/r/101275/diff/2/?file=16080#file16080line47>
I think you should change the int type here to Knm::Connection::Scope.

Ok then. I would say to move Knm::Connection::Scope to Knm::Scope, but I need to think more about that. Even if we did the move we should do it outside the scope of this patch.

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/#review3113
-----------------------------------------------------------

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 10:36 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/dc07dca0/attachment-0001.htm

Ilia Kats

2011-05-05 13:50:49 UTC

Permalink

Post by Ilia Kats

libs/internals/setting.h, line 47
<http://git.reviewboard.kde.org/r/101275/diff/2/?file=16080#file16080line47>
I think you should change the int type here to Knm::Connection::Scope.

That's just it, Knm::Connection::Scope is not yet declared here, so it does not compile. Including connection.h in setting.h yields no result either, as setting.h is included in connection.h, so that would make an infinite loop or something like that, anyway, this doesn't compile either.
Ok then. I would say to move Knm::Connection::Scope to Knm::Scope, but I need to think more about that. Even if we did the move we should do it outside the scope of this patch.

So should I commit this now and we can do the move later for everything if we're going to do that, or wait until we come up with a solution?

- Ilia

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/#review3113
-----------------------------------------------------------

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 10:36 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110505/e44c2b63/attachment.htm

Ilia Kats

2011-05-04 22:36:32 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------

(Updated May 4, 2011, 10:36 p.m.)

Review request for Network Management.

Changes
-------

Forgot to change some method calls.

Summary
-------

Currently, KDE NM is not compatible with PEM certificates (base64-encoded), which are the most used format. Attached patch makes some improvements in certificate handling, namely:
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.

This is a major change in behavior, so I would like a public review.

This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673

Diffs (updated)
-----

CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc

Diff: http://git.reviewboard.kde.org/r/101275/diff

Testing
-------

Yes, see the bugzilla ticket.

Thanks,

Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110504/be8c45cb/attachment.htm

Lamarque Vieira Souza

2011-05-05 17:16:07 UTC

Permalink

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 10:36 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110505/32c69a1a/attachment-0001.htm

Commit Hook

2011-05-05 17:54:03 UTC

Permalink

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 10:36 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110505/602cc1bc/attachment.htm

Commit Hook

2011-05-05 18:49:18 UTC

Permalink

Post by Ilia Kats
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/101275/
-----------------------------------------------------------
(Updated May 4, 2011, 10:36 p.m.)
Review request for Network Management.
Summary
-------
until now, the certificate was stored in the connection settings file, not base64-decoded or anything, and passed like that to NM. However, NM expects a binary DER certificate if using the blob scheme or a path to the certificate in the form of file://path_to_certificate if using the path scheme. Since a PEM file containing multiple certificates is possible, we would have to use the path scheme anyway, as a DER blob can only contain one certificate, so this patch also improves the certificate handling: Instead of just saving the path to the certificate, it is now being imported to $HOME/.kde/share/networkmanagement/certificates for user scope connections or /usr/share/kde4/apps/networkmanagement/certificates for systemscope connections. This allows the user to move or delete the original file without worrying about his connections. Certificates are deleted when they are no longer needed, e.g. the connection gets deleted, "Use system CA" is checked or another security method is selected.
This is a major change in behavior, so I would like a public review.
This addresses bug 209673.
http://bugs.kde.org/show_bug.cgi?id=209673
Diffs
-----
CMakeLists.txt 6748cee
backends/NetworkManager/nmdbussettingsconnectionprovider.cpp 40a364f
libs/internals/connection.h 60516dc
libs/internals/connection.cpp e00c8ac
libs/internals/connectionpersistence.cpp 2862250
libs/internals/setting.h b43365b
libs/internals/setting.cpp e66c65a
libs/internals/settings/802-1x.h 971ef41
libs/internals/settings/802-1x.cpp 245507e
libs/internals/settings/802-1xpersistence.cpp b7b9c42
libs/ui/security/eapmethodleap.cpp 199a8b8
libs/ui/security/eapmethodpeapbase.ui 4b9c5fd
libs/ui/security/eapmethodtlsbase.ui 80a5c1f
libs/ui/security/eapmethodttlsbase.ui 4f2e1a9
libs/ui/security/peapwidget.h 51ad781
libs/ui/security/peapwidget.cpp 2fc502b
libs/ui/security/tlswidget.h c71dcf3
libs/ui/security/tlswidget.cpp 1d28636
libs/ui/security/ttlswidget.h b1a9049
libs/ui/security/ttlswidget.cpp c135b19
settings/config/manageconnectionwidget.h 58afc12
settings/config/manageconnectionwidget.cpp 3c0d4dc
Diff: http://git.reviewboard.kde.org/r/101275/diff
Testing
-------
Yes, see the bugzilla ticket.
Thanks,
Ilia

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-networkmanager/attachments/20110505/de82203d/attachment.htm