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

HTTPS support for the Agama web server #1062

Merged
merged 25 commits into from
Mar 22, 2024
Merged

HTTPS support for the Agama web server #1062

merged 25 commits into from
Mar 22, 2024

Conversation

lslezak
Copy link
Contributor

@lslezak lslezak commented Feb 29, 2024

Problem

  • The communication between the user and the Agama web server needs to be encrypted

Solution

  • Enable HTTPS support using the standard openSSL library
  • Automatically redirect all external HTTP requests to HTTPS
  • Allow using HTTP locally (http://localhost)
  • Use the SSL certificate specified from the command line or generate a self-signed one

Testing

  • Tested manually

TODO

  • Update the documentation

For later:

  • Use the gethostname crate and include the current host name in the generated self-signed certificate
  • Read the SSL certificate from some default location (something like /etc/agama.d/ssl/{certificate,key}.pem ?)
  • Implement struct for handling key / certificate and associate it with relevant methods

- Use either the cerfificate specified via command line
  arguments or generate a self-signed certificate
- Redirect external HTTP requests to HTTPS
- Allow HTTP for internal connections (http://localhost)
- Optionally listen on a secondary address
  (to allow listening on both HTTP/80 and HTTPS/433 ports)
rust/package/agama.changes Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 29, 2024

Coverage Status

coverage: 74.222% (-0.3%) from 74.566%
when pulling d3870eb on https_support
into 8946391 on master.

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.

First of all, congrats for your first Rust contribution! Great work :-)

I have added a few comments, and I would like to have a closer look at the start_server as it is quite complex.

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/agama-web-server.rs Outdated Show resolved Hide resolved
rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
}

// build a SSL acceptor using a provided SSL certificate or generate a self-signed one
fn create_ssl_acceptor(cert_file: &String, key_file: &String) -> SslAcceptor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer &str to &String because it is more versatile.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, I would consider keeping both arguments together as part of the same thing:

struct Certificate { // well, I suck at naming...
  content: String,
  key: String
}

impl Certificate {
  pub read(cert: &str, key: &str) -> Result<Self>;
  pub new() -> Self; // generates the content
}

Then you can use the same code:

tls_builder.set_certificate(crt.content.as_ref());
tls_builder.set_private_key(crt.key.as_ref());

And as a bonus, you could test the certificate creation in isolation (without having anything to do with the acceptor).

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

// Generate a self-signed SSL certificate
// see https://github.com/sfackler/rust-openssl/blob/master/openssl/examples/mk_certs.rs
pub fn create_certificate() -> Result<(X509, PKey<Private>), ErrorStack> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code could be part of the Certificate struct I mentioned before.

rust/agama-server/src/agama-web-server.rs Outdated Show resolved Hide resolved
@imobachgs
Copy link
Contributor

imobachgs commented Feb 29, 2024

I did some tests, and this change seems to break the WebSocket support. At least websocat does not work (I tried ws and wss). It works :-)


let not_before = Asn1Time::days_from_now(0)?;
builder.set_not_before(&not_before)?;
let not_after = Asn1Time::days_from_now(365)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

so basically we create certificate that is valid from today for a next year. Question is are we sure we have correct date in agama? if not, then it can be issue with remote browser, that will have different date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. On the other hand the certificate is self-signed so the browser will complain anyway. Wrong date will make the problem just a little bit more serious. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use something like

// Jan 1 1970 00:00 UTC
let not_before = Asn1Time::from_unix(0)?;
// Jan 1 2040 00:00 UTC 
let not_after = Asn1Time::from_str_x509("20400101000000Z")?;

This would cover bigger range.

Anyway, I guess this will be reviewed by the security team later so this might change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, wrong date would even make troubles connecting to other remote https resources like SCC. As a solution we should probably run NTP synchronization on the Live ISO...

@imobachgs
Copy link
Contributor

I did some tests, and this change seems to break the WebSocket support. At least websocat does not work (I tried ws and wss).

That's not true, it works just fine 😅

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.

Just a few comments. It is looking better now.

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
.await
.expect("could not mount app on listener");

let ssl_acceptor = if let Ok(ssl_acceptor) = args.ssl_acceptor() {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to change it now (time pressure), but it is weird that ServeArgs is responsible for creating the SSL acceptor.

Copy link
Contributor Author

@lslezak lslezak Mar 22, 2024

Choose a reason for hiding this comment

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

If the server listens on two ports then it would be a good idea to use the same generated self-signed certificate on both ports. So the SSL acceptor needs to be created early. I added a TODO mark.

@lslezak lslezak merged commit 5416449 into master Mar 22, 2024
2 checks passed
@lslezak lslezak deleted the https_support branch March 22, 2024 16:58
@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.

5 participants