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.

Sign up for GitHub

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

Jump to bottom

Minor improvements to HTTPS handling in Agama's web server #1228

Merged
merged 17 commits into from
Jun 7, 2024
Merged

Minor improvements to HTTPS handling in Agama's web server #1228

merged 17 commits into from
Jun 7, 2024

Conversation

mchf
Copy link
Contributor

@mchf mchf commented May 17, 2024 •
edited by mvidner
Loading

Problem

Purpose is to make https support more "user" friendly. Currently self-generated certificate contains only a generic name and is generated repeatedly on each start (if needed). Also need of explicit specification of user's own certificate on each (re)start is not much friendly.

Solution

  • real hostname is written into self-signed certificate
  • generated self-signed certificate is stored for later use
  • defined default location where to search for certificates on start
  • refactoring of some old pieces. Created struct to encapsulate Certificate related stuff.

Testing

I (mvidner) have tested manually that the certificate (+key) is saved, and reused on service restart, and contains the host name.

Screenshots

agama-hostname-in-certificate

@mchf mchf requested a review from imobachgs May 22, 2024 05:19
imobachgs
imobachgs reviewed May 22, 2024
View reviewed changes
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

In general it looks good (and even better when you fix the conflict) :-) And, please, update the PR's description.

rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/cert.rs Outdated Show resolved Hide resolved
@mchf mchf force-pushed the https_enhancements branch 4 times, most recently from 8718d53 to 036f41d Compare May 23, 2024 08:02
@mchf mchf requested a review from imobachgs May 23, 2024 08:02
@imobachgs imobachgs changed the title Minor improvements to https handling in agama's web server Minor improvements to HTTPS handling in agama's web server May 23, 2024
@imobachgs imobachgs changed the title Minor improvements to HTTPS handling in agama's web server Minor improvements to HTTPS handling in Agama's web server May 23, 2024
imobachgs
imobachgs reviewed May 23, 2024
View reviewed changes
rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/cert.rs Outdated Show resolved Hide resolved
@mchf mchf requested a review from imobachgs June 4, 2024 06:48
@coveralls
Copy link

coveralls commented Jun 4, 2024 •
edited
Loading

Coverage Status

coverage: 70.368% (-0.1%) from 70.48%
when pulling f40236c on https_enhancements
into df46717 on master.

@mchf mchf force-pushed the https_enhancements branch 3 times, most recently from 5ca6a82 to ac4beba Compare June 4, 2024 07:28
imobachgs
imobachgs reviewed Jun 4, 2024
View reviewed changes
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

IMHO the code looks much better now. Just a few comments, but nothing really relevant. Nice work! Thanks!

rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/cert.rs Outdated Show resolved Hide resolved
rust/agama-server/src/cert.rs Outdated Show resolved Hide resolved
rust/agama-server/src/cert.rs Show resolved Hide resolved
rust/agama-server/src/cert.rs
builder.append_extension(
SubjectAlternativeName::new()
// use the default Agama host name
// TODO: use the gethostname crate and use the current real hostname
Copy link
Contributor

Choose a reason for hiding this comment

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

something to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well it partly is : https://github.com/openSUSE/agama/pull/1228/files#diff-280a7d38feb9a8c0c477b0903c149026ea053d231fa7ebe1326b4fd76eec07ecR62

may be @lslezak can say something about this ?

rust/package/agama.changes Show resolved Hide resolved
imobachgs
imobachgs reviewed Jun 6, 2024
View reviewed changes
service/Gemfile.lock Outdated Show resolved Hide resolved
@mchf mchf force-pushed the https_enhancements branch 5 times, most recently from 08e4e68 to 6d1d137 Compare June 6, 2024 06:36
@mchf
Copy link
Contributor Author

mchf commented Jun 6, 2024

sorry ... rebased and force pushed to get rid of of that *.swp file from the first commit

this 6d1d137 piece is new (slightly modified @mvidner's approach)

@mchf mchf requested a review from imobachgs June 6, 2024 06:39
imobachgs
imobachgs requested changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Sorry, but I still see changes in the Gemfile.lock file.

@mchf
Copy link
Contributor Author

mchf commented Jun 6, 2024

Sorry, but I still see changes in the Gemfile.lock file.

sorry ... found one more commit which contained it ... eliminated.

@mchf mchf requested a review from imobachgs June 6, 2024 08:52
@imobachgs
Copy link
Contributor

Oh, it looks like @joseivanlopez was faster and now you have a conflict to solve ;-)

mchf and others added 17 commits June 7, 2024 07:58
@mchf
Store self-generated certificate for later use
4a91eb1
@mchf
Adding hostname into certificate
0b5b27e
@mchf
Some documentation
a36daa6
@mchf
Minor refactoring, turned write_certificate into function
0d59669
@mchf
Modified and improved default way how to store self generated cert
fbd5a73
@mchf
Modified default value of cert source
ab63189
@mchf
Changed default location where to search for certificates
21db741
@mchf
Some more checks on user's input
6f3e2dd
@mchf
Cleanup based on the review
f82e247
@mchf
Updated changelog
271184d
@mchf
Refactoring. Created Certificate struct and some cleanup
6336dbe
@mchf
Fixed what was missed in rebase on master
22600a7
@mchf
Fixed formatting
70a2a39
@mchf
Tweaks based on the review
d70a618
@mchf
More changes according to the review, doc, cleanup
3e5fcea
@mvidner @mchf
Restrict permissions for the key file.
39858c4
@mchf
Refactoring code for file write and setting restricted permissions.
f40236c
@mchf
Copy link
Contributor Author

mchf commented Jun 7, 2024

Oh, it looks like @joseivanlopez was faster and now you have a conflict to solve ;-)

no problem ... just another small point into work stats ;-)

@mchf mchf merged commit 88237f7 into master Jun 7, 2024
6 checks passed
@mchf mchf deleted the https_enhancements branch June 7, 2024 06:07
@imobachgs imobachgs mentioned this pull request Jun 27, 2024
Merged
imobachgs added a commit that referenced this pull request Jun 27, 2024
@imobachgs
Update to release 9 (#1404)
8fe19db 
Prepare for releasing Agama 9. It includes the following pull requests:

- #1101
- #1202
- #1228
- #1231
- #1236
- #1238
- #1239
- #1240
- #1242
- #1243
- #1244
- #1245
- #1246
- #1247
- #1248
- #1249
- #1250
- #1251
- #1252
- #1253
- #1254
- #1255
- #1256
- #1257
- #1258
- #1259
- #1260
- #1261
- #1264
- #1265
- #1267
- #1268
- #1269
- #1270
- #1271
- #1272
- #1273
- #1274
- #1279
- #1280
- #1284
- #1285
- #1286
- #1287
- #1288
- #1289
- #1290
- #1291
- #1292
- #1293
- #1294
- #1295
- #1296
- #1298
- #1299
- #1300
- #1301
- #1302
- #1303
- #1304
- #1305
- #1306
- #1307
- #1308
- #1309
- #1310
- #1311
- #1312
- #1313
- #1314
- #1315
- #1316
- #1317
- #1318
- #1319
- #1320
- #1321
- #1322
- #1323
- #1324
- #1325
- #1326
- #1328
- #1329
- #1331
- #1332
- #1334
- #1338
- #1340
- #1341
- #1342
- #1343
- #1344
- #1345
- #1348
- #1349
- #1351
- #1352
- #1353
- #1354
- #1355
- #1356
- #1357
- #1358
- #1359
- #1360
- #1361
- #1362
- #1363
- #1365
- #1366
- #1367
- #1368
- #1371
- #1372
- #1374
- #1375
- #1376
- #1379
- #1380
- #1381
- #1383
- #1384
- #1385
- #1386
- #1387
- #1388
- #1389
- #1391
- #1392
- #1394
- #1395
- #1397
- #1398
- #1399
- #1400
- #1403
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvidner mvidner mvidner left review comments

@imobachgs imobachgs imobachgs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mchf @coveralls @imobachgs @mvidner