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

Make TPM-based encryption more explicit #995

Merged
merged 9 commits into from
Jan 18, 2024
Merged

Make TPM-based encryption more explicit #995

merged 9 commits into from
Jan 18, 2024

Conversation

ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Jan 12, 2024

Problem

Sometimes, Agama decides to use the encryption method TPM_FDE which results in the system being configured via fde-tools to open the encryption devices automatically during system boot without needing to enter the password.

That happens if the configuration parameter encryption.tpm_luks_open is set AND the system supports TPM unlocking. If that the case, the TPM_FDE encryption method is used without even asking the user. In any other case, the encryption method specified at the configuration parameter encryption.method is used.

That's all quite obscure, the users don't know whether TPM-based unlocking is going to be configured. Not even if it's possible to configure it or not.

Solution

This pull request introduces some changes in how the whole thing is managed.

Now if the system or the distribution being installed don't support TPM-based decryption, the encryption method LUKS2 is used and nothing is shown in the UI.

tpm_not_available

So no big change for the user except the fact that now LUKS2 with PBKDF2 as derivation function is the default for all distributions (it's a pretty sensible default for distributions based on Grub2 at 2024).

But if the system and the distribution both support to configure TPM-based opening of the LUKS devices, the user can choose between the TPM_FDE and the LUKS2 encryption methods via a checkbox shown in the UI.

attempt

The default encryption method (and thus, the default value of the checkbox) is configured per-product at encryption.method. If the value there is "tpm_fde" but the system does not support such a method (eg. there is no TPMv2 chip), Agama will use the default encryption method (LUKS2) as fallback.

Additionally, to make sure the user does not overlook the need to boot the machine directly to the new system in order to complete the setup, the following warning has been added to the page at the end of the installation process.

finish-tpm-b2

Expanded version:

finish-tpm-a2

Of course, if TPM encryption was not used, the hint is not there.

finish-no-tpm2

Testing

  • Tested manually
  • New unit tests for the InstallationFinished page
  • No tests added to the storage page, since it's going to heavily change soon

@coveralls
Copy link

coveralls commented Jan 12, 2024

Coverage Status

coverage: 74.865%. remained the same
when pulling 432b5b4 on tpm_fde_ui
into 35740d4 on master.

@ancorgs ancorgs force-pushed the tpm_fde_ui branch 3 times, most recently from 908b4fd to 3b6b17d Compare January 15, 2024 15:50
Comment on lines +49 to +59
/**
* Enum for the encryption method values
*
* @readonly
* @enum { string }
*/
const EncryptionMethods = Object.freeze({
LUKS2: "luks2",
TPM: "tpm_fde"
});

Copy link
Contributor

Choose a reason for hiding this comment

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

As I wrote in the commit message, following here the same approac as the current network code. We could, however, goes for what we did with client/phase or client/status instead and move it to a client/storage-encryption-methods.js or client/storage/encryption-methods.js

export const LUKS2 = "luks2";
export const TPM = "tpm_fde";

export default {
  LUKS2,
  TPM
};

and then import needed values

import { TPM } from "~/client/storage/encryption-methods"
import * as EncriptionMethods from "~/client/storage/encryption-methods"

No strong/clear opinion. Something to think about for future enhancements.

@dgdavid dgdavid marked this pull request as ready for review January 16, 2024 12:05
def try_tpm_fde?(encryption)
return false unless encryption["tpm_luks_open"] == true
def available_method?(method)
return false unless method
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: this condition should be already covered by #include?.

@dgdavid dgdavid marked this pull request as draft January 16, 2024 12:05
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

Please, add changelog.

Comment on lines +109 to +114
// Forcing the test to slow down a bit with a fake user interaction
// because actually the reminder will be not rendered immediately
// making the queryAllByText to produce a false positive if triggered
// too early here.
const congratsText = screen.getByText("Congratulations!");
await user.click(congratsText);
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, today I learned that another option for doing this might be to add a helper function in test-uitls. Something like slowDown, smallWait or whatever other name suits better

const shortBreak = async (ms = 500) => new Promise(resolve => setTimeout(resolve, ms));

and then in the test

import { shortBreak } from "test-utils";

...

await shortBreak();

I haven't tested it, just got inspiration from react-spectrum

ancorgs and others added 9 commits January 18, 2024 09:40
- Use LUKS2 with PBKDF2 as default.
- Expose the list of available methods (LUKS2 and TPM_FDE so far) in the
  D-Bus API.
For checking that conditional information is rendered when appropiate.
Even though that page will be reworked in a near future, keeping the
testsuite up-to-date worth more than its cost.
In spite of the InstallationPage needs to be reworked a bit more (and
possibly to include a loading internal state or similar), this commit
introduces minor code improvements that were cheaper to apply right now.

First of all, the TPM reminder is only shown if encryption was set.

Additionally, the reminder is improved to avoid it changing the width
when the user expands it. Done by relaying in CSS minmax function and
CSS container queries. Both with wide support of modern browsers.

This commit also stops centering vertically the page content because
the very same reason: having dinamic components make things moving
top / down depending on their state.

It also moves the TPM id to an enum created in the client/storage for
the encryption methods, following the same approach as network code.
It can be moved to an specific file like it is already done in
client/phase or client/status, though. Something to be defined/polished
in next iterations.

[1] https://developer.mozilla.org/es/docs/Web/CSS/minmax
[2] https://developer.mozilla.org/es/docs/Web/CSS/CSS_container_queries
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@ancorgs ancorgs merged commit 84c27d4 into master Jan 18, 2024
9 checks passed
@ancorgs ancorgs deleted the tpm_fde_ui branch January 18, 2024 09:04
joseivanlopez added a commit that referenced this pull request Jan 19, 2024
## Problem

The call to the sotware service to check for the availability of a
package is mocked and always returns true, assuming the package is
always available. Of course, the availability of a package depends of
the currently selected product.

## Solution

Perform a D-Bus call to the software service in order to know if a
package is available.

Note: This change exposes a problem in our services. Asking for the
product availability should be done once the software proposal is done,
otherwise the result is not reliable at all, see
#1005. For example, the TPM
option in the storage settings could not appear until the software
service has finished, see #995.

## Testing

* Added new unit tests
* Tested manually
@imobachgs imobachgs mentioned this pull request Feb 12, 2024
@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants