Skip to content

Commit

Permalink
ipsec, add phase2 dh groups for #2335
Browse files Browse the repository at this point in the history
  • Loading branch information
@AdSchellevis
AdSchellevis committed Apr 7, 2018
1 parent 3c3628c commit 28d0816
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 37 deletions.
12 changes: 0 additions & 12 deletions src/etc/inc/plugins.inc.d/ipsec.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,6 @@ $p2_protos = array(
'ah' => 'AH'
);

$p2_pfskeygroups = array(
0 => 'off',
1 => '1 (768 bit)',
2 => '2 (1024 bit)',
5 => '5 (1536 bit)',
14 => '14 (2048 bit)',
15 => '15 (3072 bit)',
16 => '16 (4096 bit)',
17 => '17 (6144 bit)',
18 => '18 (8192 bit)'
);

function ipsec_configure()
{
return array(
Expand Down
39 changes: 18 additions & 21 deletions src/www/vpn_ipsec.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,22 @@ function ipsec_idinfo_to_text(& $idinfo) {
$service_hook = 'ipsec';

include("head.inc");
$dhgroups = array(
1 => '1 (768 bits)',
2 => '2 (1024 bits)',
5 => '5 (1536 bits)',
14 => '14 (2048 bits)',
15 => '15 (3072 bits)',
16 => '16 (4096 bits)',
17 => '17 (6144 bits)',
18 => '18 (8192 bits)',
19 => '19 (256 bit elliptic curve)',
20 => '20 (384 bit elliptic curve)',
21 => '21 (521 bit elliptic curve)',
22 => '22 (1024(sub 160) bits)',
23 => '23 (2048(sub 224) bits)',
24 => '24 (2048(sub 256) bits)'
);
?>

<body>
Expand Down Expand Up @@ -399,26 +415,7 @@ function ipsec_idinfo_to_text(& $idinfo) {
}?> +

<?=strtoupper($ph1ent['hash-algorithm']);?> +

<?php
$p1_dhgroups = array(
1 => '1 (768&nbsp;bits)',
2 => '2 (1024&nbsp;bits)',
5 => '5 (1536&nbsp;bits)',
14 => '14 (2048&nbsp;bits)',
15 => '15 (3072&nbsp;bits)',
16 => '16 (4096&nbsp;bits)',
17 => '17 (6144&nbsp;bits)',
18 => '18 (8192&nbsp;bits)',
19 => '19 (256&nbsp;bit&nbsp;elliptic&nbsp;curve)',
20 => '20 (384&nbsp;bit&nbsp;elliptic&nbsp;curve)',
21 => '21 (521&nbsp;bit&nbsp;elliptic&nbsp;curve)',
22 => '22 (1024(sub 160)&nbsp;bits)',
23 => '23 (2048(sub 224)&nbsp;bits)',
24 => '24 (2048(sub 256)&nbsp;bits)'
);
?>
<?=gettext("DH Group"); ?>&nbsp;<?=$p1_dhgroups[$ph1ent['dhgroup']];?>
<?=gettext("DH Group"); ?>&nbsp;<?=$dhgroups[$ph1ent['dhgroup']];?>
</td>
<td class="hidden-xs">
<?= html_safe($p1_authentication_methods[$ph1ent['authentication_method']]['name']) ?>
Expand Down Expand Up @@ -548,7 +545,7 @@ class="act_move btn btn-<?= isset($ph2ent['disabled'])?"default":"success";?> bt
</td>
<?php
if (isset($ph2ent['pfsgroup'])): ?>
<td class="hidden-xs"><?=gettext("Group"); ?> <?=$p2_pfskeygroups[$ph2ent['pfsgroup']];?> </td>
<td class="hidden-xs"><?=gettext("Group"); ?> <?=$dhgroups[$ph2ent['pfsgroup']];?> </td>
<?php
else: ?>
<td class="hidden-xs"><?=gettext("off"); ?></td>
Expand Down
20 changes: 18 additions & 2 deletions src/www/vpn_ipsec_mobile.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,24 @@ function print_legacy_box($msg, $name, $value)
<input name="pfs_group_enable" type="checkbox" id="pfs_group_enable" value="yes" <?= !empty($pconfig['pfs_group']) ? "checked=\"checked\"" : "";?> onclick="pfs_group_change()" />

<select name="pfs_group" class="form-control" id="pfs_group">
<?php foreach ($p2_pfskeygroups as $keygroup => $keygroupname) :
?>
<?php
$p2_dhgroups = array(
1 => '1 (768 bit)',
2 => '2 (1024 bit)',
5 => '5 (1536 bit)',
14 => '14 (2048 bit)',
15 => '15 (3072 bit)',
16 => '16 (4096 bit)',
17 => '17 (6144 bit)',
18 => '18 (8192 bit)',
19 => '19 (256 bit elliptic curve)',
20 => '20 (384 bit elliptic curve)',
21 => '21 (521 bit elliptic curve)',
22 => '22 (1024(sub 160) bit)',
23 => '23 (2048(sub 224) bit)',
24 => '24 (2048(sub 256) bit)'
);
foreach ($p2_dhgroups as $keygroup => $keygroupname) :?>
<option value="<?=$keygroup;
?>" <?= $pconfig['pfs_group'] == $keygroup ? "selected=\"selected\"" : "" ; ?>>
<?=$keygroupname;?>
Expand Down
21 changes: 19 additions & 2 deletions src/www/vpn_ipsec_phase2.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ function getIndexByUniqueId($uniqid)
// initialize form data
$pconfig = array();

$phase2_fields = "ikeid,mode,descr,uniqid,proto,hash-algorithm-option,pfsgroup,pfsgroup,lifetime,pinghost,protocol,spd";
$phase2_fields = "ikeid,mode,descr,uniqid,proto,hash-algorithm-option,pfsgroup,lifetime,pinghost,protocol,spd";
if ($p2index !== null) {
// 1-on-1 copy
foreach (explode(",", $phase2_fields) as $fieldname) {
Expand Down Expand Up @@ -673,7 +673,24 @@ function getIndexByUniqueId($uniqid)
if (!isset($pconfig['mobile']) || !isset($config['ipsec']['client']['pfs_group'])) :?>
<select name="pfsgroup">
<?php
foreach ($p2_pfskeygroups as $keygroup => $keygroupname) :?>
$p2_dhgroups = array(
1 => '1 (768 bit)',
2 => '2 (1024 bit)',
5 => '5 (1536 bit)',
14 => '14 (2048 bit)',
15 => '15 (3072 bit)',
16 => '16 (4096 bit)',
17 => '17 (6144 bit)',
18 => '18 (8192 bit)',
19 => '19 (256 bit elliptic curve)',
20 => '20 (384 bit elliptic curve)',
21 => '21 (521 bit elliptic curve)',
22 => '22 (1024(sub 160) bit)',
23 => '23 (2048(sub 224) bit)',
24 => '24 (2048(sub 256) bit)'
);

foreach ($p2_dhgroups as $keygroup => $keygroupname) :?>
<option value="<?=$keygroup;?>" <?= $keygroup == $pconfig['pfsgroup'] ? "selected=\"selected\"" : "";?>>
<?=$keygroupname;?>
</option>
Expand Down

9 comments on commit 28d0816

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

A single getter function in ipsec.inc would be favourable. Openvpn does that nowadays, it also removes side-effects from ipsec.inc 😊

@AdSchellevis
Copy link
Member Author

Choose a reason for hiding this comment

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

@fichtner I rather keep the ipsec.inc cleaner at the moment, this code is only for UI rendering, which keeps it easier to inspect the ipsec.inc which we will need to refactor at some point in time. I know, having the same list twice isn't optimal, but for now it helps to clear the view for ipsec.inc.

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

sure, ideally we need an ui renderer and a backend file, but the values mandated by the ui need to fit the backend anyway. OpenVPN was cleaned up in this way... 86989c2#diff-b42a80557c087fb143adf379de337801 ... just want to drop it here for reference

@mimugmail
Copy link
Member

Choose a reason for hiding this comment

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

With this change to choice to disable DH is gone, see:
https://forum.opnsense.org/index.php?topic=8643.msg38513;boardseen#new

Seems esp. some older clients need this.

@AdSchellevis
Copy link
Member Author

Choose a reason for hiding this comment

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

@mimugmail thanks, I'll prepare a fix right away

@AdSchellevis
Copy link
Member Author

Choose a reason for hiding this comment

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

@mimugmail e0cc1c5 should add the option again.

@mimugmail
Copy link
Member

Choose a reason for hiding this comment

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

Seems P1 also needs a fix. P2 works now

@AdSchellevis
Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in next commit

@mimugmail
Copy link
Member

Choose a reason for hiding this comment

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

Works, thank you! 👍

Please sign in to comment.