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

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

Merged
merged 17 commits into from
Jun 7, 2024

Conversation

mchf
Copy link
Contributor

@mchf mchf commented May 17, 2024

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

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
rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/cert.rs Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 4, 2024

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
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
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.

rust/package/agama.changes Show resolved Hide resolved
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
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
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
imobachgs added a commit that referenced this pull request Jun 27, 2024
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants