* [pbs-devel] Preperation patches for access/auth log
@ 2020-10-15 15:49 Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits Thomas Lamprecht
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Thomas Lamprecht @ 2020-10-15 15:49 UTC (permalink / raw)
To: pbs-devel
Those patches were made when implementing the access log for the API requests,
but as that needs still a bit work regarding log-rotation (and I'm a bit slow
as I've to jump to another thing every 10 minutes or so) , I sent out the
rather independent stuff first.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits
2020-10-15 15:49 [pbs-devel] Preperation patches for access/auth log Thomas Lamprecht
@ 2020-10-15 15:49 ` Thomas Lamprecht
2020-10-16 8:45 ` [pbs-devel] applied: " Dietmar Maurer
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 2/4] server: rest: also log the query part of URL Thomas Lamprecht
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Thomas Lamprecht @ 2020-10-15 15:49 UTC (permalink / raw)
To: pbs-devel
Add a generous limit now and return the correct error (414 URI Too
Long). Otherwise we could to pretty larger GET requests, 64 KiB and
possible bigger (at 64 KiB my simple curl test failed due to
shell/curl limitations).
For now allow a 3072 characters as combined length of URI path and
query.
This is conform with the HTTP/1.1 RFCs (e.g., RFC 7231, 6.5.12 and
RFC 2616, 3.2.1) which do not specify any limits, upper or lower, but
require that all server accessible resources mus be reachable without
getting 414, which is normally fulfilled as we have various length
limits for stuff which could be in an URI, in place, e.g.:
* user id: max. 64 chars
* datastore: max. 32 chars
The only known problematic API endpoint is the catalog one, used in
the GUI's pxar file browser:
GET /api2/json/admin/datastore/<id>/catalog?..&filepath=<path>
The <path> is the encoded archive path, and can be arbitrary long.
But, this is a flawed design, as even without this new limit one can
easily generate archives which cannot be browsed anymore, as hyper
only accepts requests with max. 64 KiB in the URI.
So rather, we should move that to a GET-as-POST call, which has no
such limitations (and would not need to base32 encode the path).
Note: This change was inspired by adding a request access log, which
profits from such limits as we can then rely on certain atomicity
guarantees when writing requests to the log.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
talked about this with a few people here. Follow ups which would be nice are:
* move getting the snapshuts catalog from get to post, fixing the issue with
passing along a file path
* add checks for longest API (+ query) path in verify API, giving some more
guarantees about the used limits.
src/server/rest.rs | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/server/rest.rs b/src/server/rest.rs
index 98e56039..55a42ac5 100644
--- a/src/server/rest.rs
+++ b/src/server/rest.rs
@@ -52,6 +52,8 @@ pub struct RestServer {
pub api_config: Arc<ApiConfig>,
}
+const MAX_URI_QUERY_LENGTH: usize = 3072;
+
impl RestServer {
pub fn new(api_config: ApiConfig) -> Self {
@@ -115,6 +117,10 @@ fn log_response(
if resp.extensions().get::<NoLogExtension>().is_some() { return; };
+ // we also log URL-to-long requests, so avoid message bigger than PIPE_BUF (4k on Linux)
+ // to profit from atomicty guarantees for O_APPEND opened logfiles
+ let path = &path[..MAX_URI_QUERY_LENGTH.min(path.len())];
+
let status = resp.status();
if !(status.is_success() || status.is_informational()) {
@@ -532,12 +538,19 @@ async fn handle_request(
) -> Result<Response<Body>, Error> {
let (parts, body) = req.into_parts();
-
let method = parts.method.clone();
let (path, components) = tools::normalize_uri_path(parts.uri.path())?;
let comp_len = components.len();
+ let query = parts.uri.query().unwrap_or_default();
+ if path.len() + query.len() > MAX_URI_QUERY_LENGTH {
+ return Ok(Response::builder()
+ .status(StatusCode::URI_TOO_LONG)
+ .body("".into())
+ .unwrap());
+ }
+
let env_type = api.env_type();
let mut rpcenv = RestEnvironment::new(env_type);
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH backup 2/4] server: rest: also log the query part of URL
2020-10-15 15:49 [pbs-devel] Preperation patches for access/auth log Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits Thomas Lamprecht
@ 2020-10-15 15:49 ` Thomas Lamprecht
2020-10-16 8:46 ` [pbs-devel] applied: " Dietmar Maurer
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 4/4] tools: file logger: allow more control over file creation and log format Thomas Lamprecht
3 siblings, 1 reply; 10+ messages in thread
From: Thomas Lamprecht @ 2020-10-15 15:49 UTC (permalink / raw)
To: pbs-devel
As it is part of the request and we do so in our other products
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
src/server/rest.rs | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/server/rest.rs b/src/server/rest.rs
index 55a42ac5..7305643c 100644
--- a/src/server/rest.rs
+++ b/src/server/rest.rs
@@ -111,7 +111,7 @@ pub struct ApiService {
fn log_response(
peer: &std::net::SocketAddr,
method: hyper::Method,
- path: &str,
+ path_query: &str,
resp: &Response<Body>,
) {
@@ -119,7 +119,7 @@ fn log_response(
// we also log URL-to-long requests, so avoid message bigger than PIPE_BUF (4k on Linux)
// to profit from atomicty guarantees for O_APPEND opened logfiles
- let path = &path[..MAX_URI_QUERY_LENGTH.min(path.len())];
+ let path = &path_query[..MAX_URI_QUERY_LENGTH.min(path_query.len())];
let status = resp.status();
@@ -156,7 +156,7 @@ impl tower_service::Service<Request<Body>> for ApiService {
}
fn call(&mut self, req: Request<Body>) -> Self::Future {
- let path = req.uri().path().to_owned();
+ let path = req.uri().path_and_query().unwrap().as_str().to_owned();
let method = req.method().clone();
let config = Arc::clone(&self.api_config);
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior
2020-10-15 15:49 [pbs-devel] Preperation patches for access/auth log Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 2/4] server: rest: also log the query part of URL Thomas Lamprecht
@ 2020-10-15 15:49 ` Thomas Lamprecht
2020-10-16 8:44 ` Dietmar Maurer
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 4/4] tools: file logger: allow more control over file creation and log format Thomas Lamprecht
3 siblings, 1 reply; 10+ messages in thread
From: Thomas Lamprecht @ 2020-10-15 15:49 UTC (permalink / raw)
To: pbs-devel
will be extended in the next patch
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
src/server/worker_task.rs | 7 +++++--
src/tools/file_logger.rs | 35 +++++++++++++++++++++++++----------
2 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/src/server/worker_task.rs b/src/server/worker_task.rs
index bd19782c..fc41b52a 100644
--- a/src/server/worker_task.rs
+++ b/src/server/worker_task.rs
@@ -21,7 +21,7 @@ use proxmox::tools::fs::{create_path, open_file_locked, replace_file, CreateOpti
use super::UPID;
use crate::tools::logrotate::{LogRotate, LogRotateFiles};
-use crate::tools::FileLogger;
+use crate::tools::{FileLogger, FileLogOptions};
use crate::api2::types::Userid;
macro_rules! PROXMOX_BACKUP_VAR_RUN_DIR_M { () => ("/run/proxmox-backup") }
@@ -672,7 +672,10 @@ impl WorkerTask {
println!("FILE: {:?}", path);
- let logger = FileLogger::new(&path, to_stdout)?;
+ let logger_options = FileLogOptions {
+ to_stdout: to_stdout,
+ };
+ let logger = FileLogger::new(&path, logger_options)?;
nix::unistd::chown(&path, Some(backup_user.uid), Some(backup_user.gid))?;
let worker = Arc::new(Self {
diff --git a/src/tools/file_logger.rs b/src/tools/file_logger.rs
index 48a0bae2..69dd0bd4 100644
--- a/src/tools/file_logger.rs
+++ b/src/tools/file_logger.rs
@@ -17,11 +17,17 @@ use std::io::Write;
/// flog!(log, "A simple log: {}", "Hello!");
/// ```
+#[derive(Debug, Default)]
+/// Options to control the behavior of a ['FileLogger'] instance
+pub struct FileLogOptions {
+ /// Duplicate logged messages to STDOUT, like tee
+ pub to_stdout: bool,
+}
#[derive(Debug)]
pub struct FileLogger {
file: std::fs::File,
- to_stdout: bool,
+ options: FileLogOptions,
}
/// Log messages to [FileLogger](tools/struct.FileLogger.html)
@@ -33,23 +39,24 @@ macro_rules! flog {
}
impl FileLogger {
-
- pub fn new<P: AsRef<std::path::Path>>(file_name: P, to_stdout: bool) -> Result<Self, Error> {
-
+ pub fn new<P: AsRef<std::path::Path>>(
+ file_name: P,
+ options: FileLogOptions,
+ ) -> Result<Self, Error> {
let file = std::fs::OpenOptions::new()
.read(true)
.write(true)
.create_new(true)
.open(file_name)?;
- Ok(Self { file , to_stdout })
+ Ok(Self { file, options })
}
pub fn log<S: AsRef<str>>(&mut self, msg: S) {
let msg = msg.as_ref();
- let mut stdout = std::io::stdout();
- if self.to_stdout {
+ if self.options.to_stdout {
+ let mut stdout = std::io::stdout();
stdout.write_all(msg.as_bytes()).unwrap();
stdout.write_all(b"\n").unwrap();
}
@@ -57,19 +64,27 @@ impl FileLogger {
let now = proxmox::tools::time::epoch_i64();
let rfc3339 = proxmox::tools::time::epoch_to_rfc3339(now).unwrap();
- let line = format!("{}: {}\n", rfc3339, msg);
+ let line = if self.options.prefix_time {
+ format!("{}: {}\n", rfc3339, msg)
+ } else {
+ format!("{}\n", msg)
+ };
self.file.write_all(line.as_bytes()).unwrap();
}
}
impl std::io::Write for FileLogger {
fn write(&mut self, buf: &[u8]) -> Result<usize, std::io::Error> {
- if self.to_stdout { let _ = std::io::stdout().write(buf); }
+ if self.options.to_stdout {
+ let _ = std::io::stdout().write(buf);
+ }
self.file.write(buf)
}
fn flush(&mut self) -> Result<(), std::io::Error> {
- if self.to_stdout { let _ = std::io::stdout().flush(); }
+ if self.options.to_stdout {
+ let _ = std::io::stdout().flush();
+ }
self.file.flush()
}
}
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH backup 4/4] tools: file logger: allow more control over file creation and log format
2020-10-15 15:49 [pbs-devel] Preperation patches for access/auth log Thomas Lamprecht
` (2 preceding siblings ...)
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior Thomas Lamprecht
@ 2020-10-15 15:49 ` Thomas Lamprecht
2020-10-16 8:51 ` [pbs-devel] applied: " Dietmar Maurer
3 siblings, 1 reply; 10+ messages in thread
From: Thomas Lamprecht @ 2020-10-15 15:49 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
src/server/worker_task.rs | 3 +++
src/tools/file_logger.rs | 15 +++++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/server/worker_task.rs b/src/server/worker_task.rs
index fc41b52a..f3846596 100644
--- a/src/server/worker_task.rs
+++ b/src/server/worker_task.rs
@@ -674,6 +674,9 @@ impl WorkerTask {
let logger_options = FileLogOptions {
to_stdout: to_stdout,
+ exclusive: true,
+ read: true,
+ ..Default::default()
};
let logger = FileLogger::new(&path, logger_options)?;
nix::unistd::chown(&path, Some(backup_user.uid), Some(backup_user.gid))?;
diff --git a/src/tools/file_logger.rs b/src/tools/file_logger.rs
index 69dd0bd4..2f406c0d 100644
--- a/src/tools/file_logger.rs
+++ b/src/tools/file_logger.rs
@@ -20,8 +20,17 @@ use std::io::Write;
#[derive(Debug, Default)]
/// Options to control the behavior of a ['FileLogger'] instance
pub struct FileLogOptions {
+ /// Open underlying log file in append mode, useful when multiple concurrent
+ /// writers log to the same file. For example, an HTTP access log.
+ pub append: bool,
+ /// Open underlying log file as readable
+ pub read: bool,
+ /// If set, ensure that the file is newly created or error out if already existing.
+ pub exclusive: bool,
/// Duplicate logged messages to STDOUT, like tee
pub to_stdout: bool,
+ /// Prefix messages logged to the file with the current local time as RFC 3339
+ pub prefix_time: bool,
}
#[derive(Debug)]
@@ -44,9 +53,11 @@ impl FileLogger {
options: FileLogOptions,
) -> Result<Self, Error> {
let file = std::fs::OpenOptions::new()
- .read(true)
+ .read(options.read)
.write(true)
- .create_new(true)
+ .append(options.append)
+ .create_new(options.exclusive)
+ .create(!options.exclusive)
.open(file_name)?;
Ok(Self { file, options })
--
2.27.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior Thomas Lamprecht
@ 2020-10-16 8:44 ` Dietmar Maurer
2020-10-16 8:46 ` Thomas Lamprecht
0 siblings, 1 reply; 10+ messages in thread
From: Dietmar Maurer @ 2020-10-16 8:44 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Thomas Lamprecht
> - let line = format!("{}: {}\n", rfc3339, msg);
> + let line = if self.options.prefix_time {
> + format!("{}: {}\n", rfc3339, msg)
> + } else {
> + format!("{}\n", msg)
> + };
This breaks the build (there is no prefix-time)
error[E0609]: no field `prefix_time` on type `tools::file_logger::FileLogOptions`
--> src/tools/file_logger.rs:67:36
|
67 | let line = if self.options.prefix_time {
| ^^^^^^^^^^^ unknown field
|
= note: available fields are: `to_stdout`
error: aborting due to previous error
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] applied: [PATCH backup 1/4] server: rest: implement max URI path and query length request limits
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits Thomas Lamprecht
@ 2020-10-16 8:45 ` Dietmar Maurer
0 siblings, 0 replies; 10+ messages in thread
From: Dietmar Maurer @ 2020-10-16 8:45 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Thomas Lamprecht
applied
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] applied: [PATCH backup 2/4] server: rest: also log the query part of URL
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 2/4] server: rest: also log the query part of URL Thomas Lamprecht
@ 2020-10-16 8:46 ` Dietmar Maurer
0 siblings, 0 replies; 10+ messages in thread
From: Dietmar Maurer @ 2020-10-16 8:46 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Thomas Lamprecht
applied
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior
2020-10-16 8:44 ` Dietmar Maurer
@ 2020-10-16 8:46 ` Thomas Lamprecht
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Lamprecht @ 2020-10-16 8:46 UTC (permalink / raw)
To: Dietmar Maurer, Proxmox Backup Server development discussion
On 16.10.20 10:44, Dietmar Maurer wrote:
>
>> - let line = format!("{}: {}\n", rfc3339, msg);
>> + let line = if self.options.prefix_time {
>> + format!("{}: {}\n", rfc3339, msg)
>> + } else {
>> + format!("{}\n", msg)
>> + };
>
> This breaks the build (there is no prefix-time)
>
> error[E0609]: no field `prefix_time` on type `tools::file_logger::FileLogOptions`
> --> src/tools/file_logger.rs:67:36
> |
> 67 | let line = if self.options.prefix_time {
> | ^^^^^^^^^^^ unknown field
> |
> = note: available fields are: `to_stdout`
>
> error: aborting due to previous error
>
argh, sorry, to much git rebasing :(
I'll send a v2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] applied: [PATCH backup 4/4] tools: file logger: allow more control over file creation and log format
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 4/4] tools: file logger: allow more control over file creation and log format Thomas Lamprecht
@ 2020-10-16 8:51 ` Dietmar Maurer
0 siblings, 0 replies; 10+ messages in thread
From: Dietmar Maurer @ 2020-10-16 8:51 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Thomas Lamprecht
merged with patch 3/4
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-16 8:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 15:49 [pbs-devel] Preperation patches for access/auth log Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 1/4] server: rest: implement max URI path and query length request limits Thomas Lamprecht
2020-10-16 8:45 ` [pbs-devel] applied: " Dietmar Maurer
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 2/4] server: rest: also log the query part of URL Thomas Lamprecht
2020-10-16 8:46 ` [pbs-devel] applied: " Dietmar Maurer
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 3/4] tools: file logger: use option struct to control behavior Thomas Lamprecht
2020-10-16 8:44 ` Dietmar Maurer
2020-10-16 8:46 ` Thomas Lamprecht
2020-10-15 15:49 ` [pbs-devel] [PATCH backup 4/4] tools: file logger: allow more control over file creation and log format Thomas Lamprecht
2020-10-16 8:51 ` [pbs-devel] applied: " Dietmar Maurer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox