Optimize header cache RAM layout, add tests for cache hit/miss

README.md

@@ -53,10 +53,11 @@ The `ldap_return_attribs`, if not empty, specifies a comma-separated list of LDA
 attributes to return to Nginx in HTTP headers. The header names are prefixed with
 `X-Ldap-Authz-Res-`, so for example `displayName` attribute is returned in
 `X-Ldap-Authz-Res-displayName` header. Use `ldap_return_attribs = *` to return all
-attributes (mainly useful for debugging).
+attributes (mainly useful for debugging). Attributes with multiple values are
+concatenated with `;` separator.
 
-If LDAP query returns multiple results, the first one is used. To see all results,
-use `--debug` option to write them to log.
+If LDAP query returns multiple objects, the first one is used. To see the rest,
+use `--debug` option to log them.
 
 ## Cache
 
@@ -70,6 +71,9 @@ A cache entry takes probably about 1kB of RAM, unless you requested all LDAP att
 Technically, each config section gets its own cache, so you can have different cache sizes and
 retention times for different sections.
 
+HTTP response headers contain `X-Ldap-Cached` header that is set to `1` if the response
+was served from cache, and `0` if it was a fresh query.
+
 ## Building
 
 The server is written in Rust and can be built with `cargo build --release`.
@@ -214,7 +218,7 @@ sed -i 's@ldap_server_url *=.*@ldap_server_url = ldap://127.0.0.1:3890@' example
 cargo run -- example.ini --debug &
 
 # Test request directly against ldap_authz_proxy
-curl http://127.0.0.1:10567/admins -H "X-Ldap-Authz-Username:alice"
+curl http://127.0.0.1:10567/admins -H "X-Ldap-Authz-Username:alice" -I
 
 # Cleanup
 kill %1  # Or do: fg + ctrl-c

run-tests.sh

@@ -8,10 +8,7 @@ echo "--------------------------------------------------------------------------
 cd test
 DOCKER_BUILDKIT=1 docker compose build
 docker compose up -d
-
-# Make sure we stop the containers when we exit, regardless of how
-trap 'echo "---"; docker compose down' EXIT
-
+trap 'echo "---"; docker compose down' EXIT  #  Make sure we stop the containers when we exit, regardless of how
 
 echo "---------------------------------------------"
 echo "Waiting for services to start up..."
@@ -47,10 +44,13 @@ echo "---------------------------------------------"
 function request() {
     FOLDER="$1"
     CREDS="$2"
-    RES=$(curl -s http://127.0.0.1:8090/$FOLDER/ -u "$CREDS" -I)
+    # Don't remove the "/index.html", otherwise Nginx makes an internal redirect, and warms up LDAP cache prematurely
+    RES=$(curl -s http://127.0.0.1:8090/$FOLDER/index.html -u "$CREDS" -I)
+    # cat <<< """$RES""" >&2
     HTTP_CODE=$(grep HTTP <<< """$RES""" | awk '{print $2}' | tr -d '\r\n')
-    DISPLAY_NAME=$(grep 'X-Display-Name' <<< """$RES""" | sed 's/^.*: //' | tr -d '\r\n') || true
-    echo "${HTTP_CODE}${DISPLAY_NAME}"
+    DISPLAY_NAME=$(grep -i 'X-Display-Name' <<< """$RES""" | sed 's/^.*: //' | tr -d '\r\n') || true
+    LDAP_CACHED=$(grep -i 'X-LDAP-Cached' <<< """$RES""" | sed 's/^.*: //' | tr -d '\r\n') || true
+    echo "${HTTP_CODE}${DISPLAY_NAME} c${LDAP_CACHED}"
 }
 
 function test() {
@@ -59,24 +59,29 @@ function test() {
     EXPECTED="$3"
     ACTUAL="$(request $FOLDER $CREDS)"
     if [ "$ACTUAL" != "$EXPECTED" ]; then
-        echo "Test FAILED - expected $EXPECTED, got $ACTUAL for '$FOLDER' with '$CREDS'"
+        echo "Test FAILED - expected '$EXPECTED', got '$ACTUAL' for '$FOLDER' with '$CREDS'"
     else
         echo "Test OK for '$FOLDER' with '$CREDS' ($ACTUAL == $EXPECTED)"
     fi
 }
 
 function do_tests() {
-    test "user-page" "alice:alice" "200Alice Alison"
-    test "admin-page" "alice:alice" "200"
-    test "user-page" "alice:BADPASSWORD" "401"
-    test "admin-page" "alice:BADPASSWORD" "401"
+    test "user-page" "alice:alice" "200Alice Alison c0"
+    test "admin-page" "alice:alice" "200 c0"
+    test "user-page" "alice:BADPASSWORD" "401 c"
+    test "admin-page" "alice:BADPASSWORD" "401 c"
 
-    test "user-page" "bob:bob" "200Bob Bobrikov"
-    test "admin-page" "bob:bob" "403"
-    test "user-page" "bob:BADPASSWORD" "401"
-    test "admin-page" "bob:BADPASSWORD" "401"
+    test "user-page" "bob:bob" "200Bob Bobrikov c0"
+    test "admin-page" "bob:bob" "403 c"
+    test "user-page" "bob:BADPASSWORD" "401 c"
+    test "admin-page" "bob:BADPASSWORD" "401 c"
 
-    test "bad-page" "alice:alice" "404"
+    test "bad-page" "alice:alice" "404 c"
+
+    echo "(Repeat and check that query came from cache)"
+    test "user-page" "alice:alice" "200Alice Alison c1"
+    test "admin-page" "alice:alice" "200 c1"
+    test "user-page" "bob:bob" "200Bob Bobrikov c1"
 }
 
 # Run the tests and summarize

src/main.rs

@@ -41,7 +41,7 @@ Options:
 "#;
 
 type Sha256Hash = sha2::digest::generic_array::GenericArray<u8, sha2::digest::generic_array::typenum::U32>;
-type LdapSearchRes = Option<HashMap<String, String>>; // HashMap<attr_name, attr_value> or None if not found
+type LdapSearchRes = Option<Vec<(String, String)>>; // HashMap<attr_name, attr_value> or None if not found
 type LdapCache = LruCache<Sha256Hash, LdapSearchRes>;
 
 struct ReqContext {
@@ -59,7 +59,7 @@ struct LdapAnswer {
 /// Caches the result for the next time.
 ///
 /// TODO: Pool connections. Currently a new LDAP connection is created for every request unless cache is hit.
-async fn ldap_check(conf: &ConfigSection, username: &str, cache: &Arc<Mutex<LdapCache>>) -> ldap3::result::Result<LdapAnswer> {
+async fn ldap_query(conf: &ConfigSection, username: &str, cache: &Arc<Mutex<LdapCache>>) -> ldap3::result::Result<LdapAnswer> {
     let query = conf.ldap_query.replace("%USERNAME%", &ldap3::ldap_escape(username));
     tracing::debug!("LDAP string: {}", query);
 
@@ -102,16 +102,16 @@ async fn ldap_check(conf: &ConfigSection, username: &str, cache: &Arc<Mutex<Ldap
 
     // Store first row in a HashMap and log all other rows
     let row_i = 0;
-    let mut attribs = HashMap::new();
+    let mut attribs = Vec::new();
     for row in rs {
         let se = SearchEntry::construct(row);
         if row_i > 0 {
             tracing::debug!("Skipped additional result row #{}: {:?}", row_i, se);
         } else {
             tracing::debug!("First result row: {:?}", se);
             for (key, vals) in se.attrs {
-                let vals_comb = vals.iter().map(|v| v.as_str()).collect::<Vec<&str>>().join(", ");
-                attribs.insert(key, vals_comb);
+                let vals_comb = vals.iter().map(|v| v.as_str()).collect::<Vec<&str>>().join(";");
+                attribs.push((key, vals_comb));
             }
         }
     }
@@ -168,17 +168,16 @@ async fn http_handler(req: Request<Body>, ctx: Arc<ReqContext>) -> Result<Respon
 
     // Check LDAP (and cache)
     let cache = ctx.cache.get(conf.section.as_str()).unwrap();
-    match ldap_check(&conf, username, cache).await {
+    match ldap_query(&conf, username, cache).await {
         Err(e) => {
             span.in_scope(|| { tracing::error!("LDAP error: {:?}", e); });
             return Response::builder().status(StatusCode::BAD_GATEWAY).body(Body::from("LDAP error"))
         },
         Ok(la) => {
             let span = span.record("cached", &la.cached);
-            return if let Some(ldap_res) = la.ldap_res {
+            if let Some(ldap_res) = la.ldap_res {
                 span.in_scope(|| { tracing::info!("User authorized Ok"); });
                 let mut resp = Response::new(Body::from("200 OK - LDAP result found"));
-
                 // Store LDAP result attributes to response HTTP headers
                 for (key, val) in ldap_res.iter() {
                     let hname = match HeaderName::from_str(format!("X-LDAP-RES-{}", key).as_str()) {
@@ -198,10 +197,12 @@ async fn http_handler(req: Request<Body>, ctx: Arc<ReqContext>) -> Result<Respon
                     span.in_scope(|| { tracing::debug!("Adding result HTTP header: {:?} = {:?}", hname, hval); });
                     resp.headers_mut().insert(hname, hval);
                 }
+                resp.headers_mut().insert("X-LDAP-CACHED", HeaderValue::from_str(if la.cached { "1" } else { "0" }).unwrap());
                 Ok(resp)
             } else {
-                span.in_scope(|| { tracing::info!("User REJECTED"); });
-                Response::builder().status(StatusCode::FORBIDDEN).body(Body::from(format!("403 Forbidden - Empty LDAP result for user '{}'", username)))
+                Response::builder().status(StatusCode::FORBIDDEN)
+                .header("X-LDAP-CACHED", HeaderValue::from_str(if la.cached { "1" } else { "0" }).unwrap())
+                .body(Body::from(format!("403 Forbidden - Empty LDAP result for user '{:?}'", username)))
             }
         }
     }

test/Dockerfile

@@ -14,6 +14,7 @@ RUN set -eux; \
 COPY test/nginx-site.conf /etc/nginx/sites-available/default
 RUN htpasswd -cb /var/www/html/.htpasswd alice alice
 RUN htpasswd -b /var/www/html/.htpasswd bob bob
+RUN cp -a /var/www/html/index.nginx-debian.html /var/www/html/index.html
 
 WORKDIR /run
 

test/nginx-site.conf

@@ -28,22 +28,27 @@ server {
        	       	auth_request   	/authz_users;
                 auth_request_set $display_name $upstream_http_x_ldap_res_displayname;
                 auth_request_set $email $upstream_http_x_ldap_res_mail;
+                auth_request_set $ldap_cached $upstream_http_x_ldap_cached;
 
        	       	alias /var/www/html;
-       	       	index index.nginx-debian.html;
+       	       	index index.html;
 
                 add_header X-Display-Name $display_name;
                 add_header X-Email $email;
+                add_header X-Ldap-Cached $ldap_cached;
        	}
         location /admin-page {
        	       	satisfy all;
 
                 auth_basic "Admin area";
                 auth_basic_user_file /var/www/html/.htpasswd;
+                auth_request_set $ldap_cached $upstream_http_x_ldap_cached;
 
                 auth_request    /authz_admins;
 
        	       	alias /var/www/html;
-       	       	index index.nginx-debian.html;
+       	       	index index.html;
+
+                add_header X-Ldap-Cached $ldap_cached;
         }
 }