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

freeradius: show ECDSA CAs and certs only with correct curves #710

Merged
merged 4 commits into from
Nov 25, 2019

Conversation

vktg
Copy link
Contributor

@vktg vktg commented Nov 16, 2019

Redmine Issue: https://redmine.pfsense.org/issues/9906
Ready for review

Do not show incompatible ECDSA CAs or certs for FreeRADIUS
same as https://redmine.pfsense.org/issues/9897

Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

This seems OK but do we really know it has this limitation? Is there anything documented saying it wouldn't work, or that it has problems? For example, OpenVPN can apparently work with any curve so long as both sides agree on it and it's supported by their local SSL libraries. I'm hesitant to merge this unless we know it's necessary.

And it seems like since both FreeRADIUS and IPsec are using it for EAP/TLS, that using the IPsec list is a better fit, but again, even that might not be necessary.

@vktg
Copy link
Contributor Author

vktg commented Nov 21, 2019

Yes, it seems that using the IPsec list is safe

See https://www.sevecek.com/EnglishPages/Lists/Posts/Post.aspx?ID=57 about Windows Suite-B support
and https://docs.microsoft.com/en-us/windows/security/threat-protection/windows-platform-common-criteria , where you can see only “NIST curves” P-256, P-384 and P-521 for latest Windows versions

$ecdsagood = cert_build_list('ca', 'IPsec');
} else {
$ecdsagood = cert_build_list('cert', 'IPsec');
$c_arr[] = array('refid' => 'none', 'descr' => 'none (auto)');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only added to the certificate array? Before it was added to both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, fixed

}
function freeradius_get_ca_or_certs($type) {
$c_arr = array();
if ($type == 'ca') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified a bit since you are passing the same string as $type on to cert_build_list(), so the whole if block can be changed to something like:

$c_arr = array();
$c_arr[] = array('refid' => 'none', 'descr' => 'none (auto)');
$ecdsagood = cert_build_list($type, 'IPsec');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@netgate-git-updates netgate-git-updates merged commit e4c1b63 into pfsense:devel Nov 25, 2019
netgate-git-updates pushed a commit that referenced this pull request Nov 27, 2021
Changelog:
	* Fixed: Version `Qt_5.15' not found (required by
	  /usr/bin/ksnip). (#712)
	* Fixed: CI packages show continuous suffix for tagged build.
	  (#710)
	* Fixed: kImageAnnotator not translated with deb package. (#359)
	* Fixed: Windows packages increased in size. (#713)
	* Fixed: The string 'Actions' is not available for translation.
	  (#729)
	* Fixed: HiDPI issue with multiple screen on Windows. (#668)
	* Fixed: Snipping Area not closing when pressing ESC. (#735)
	* Fixed: Sometimes "Snipping Area Rulers" not shown after
	  starting rectangular selection. (#684)
	* Fixed: Cursor not positioned correctly when snipping area
	  opens. (#736)
	* Fixed: Mouse cursor not captured when triggered via global
	  shortcut. (#737)
	* Fixed: Dual 4K screens get scrambled on X11. (#734)
	* Fixed: VCRUNTIME140_1.dll was not found. (#743)
	* Fixed: Screenshot area issue when monitor count changes on
	  Windows. (#722)
	* Fixed: Wayland does not support QWindow::requestActivate().
	  (#656)
	* Fixed: Wrong area is captured on a Wayland screen scaling.
	  (#691)
	* Changed: Enforce xdg-desktop-portal screenshots for Gnome >=
	  41. (#727)
	* Fixed kImageAnnotator: Crash while typing text on wayland.
	  (#256)
	* Changed kImageAnnotator: Show scrollbar when not all tools
	  visible. (#258)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants