[change] Made Site name configurable and updated OpenVPN template conf#566
[change] Made Site name configurable and updated OpenVPN template conf#566nemesifier merged 5 commits intomasterfrom
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
pandafy
moved this from To do (general)
to Needs review
in OpenWISP Contributor's Board
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces Template.objects.create(...) with constructing Template(...), calling full_clean(), conditionally setting config["openvpn"][0]["log"] = "/var/log/tun0.log" when an OpenVPN config is present, then saving. Adds update_default_site() which reads DASHBOARD_DOMAIN and updates the Django Sites default entry when the existing site name or domain equals "example.com". Calls to update_default_site() were added after admin creation in create_default_topology(vpn) and in the script main flow. openvpn.json now sets "cipher": "AES-128-GCM" and adds "data_ciphers" and "data_ciphers_fallback" with "AES-128-GCM:none". Sequence Diagram(s)sequenceDiagram
participant Script as Script
participant Template as TemplateModel
participant Admin as AdminCreator
participant Env as Env (DASHBOARD_DOMAIN)
participant Sites as Sites DB
Script->>Template: instantiate Template(...)\ncall full_clean()
Template->>Template: if config.openvpn exists\nset config.openvpn[0].log = "/var/log/tun0.log"
Template->>Template: save()
Script->>Admin: create admin user
Admin-->>Script: admin created
Script->>Env: read DASHBOARD_DOMAIN
Script->>Sites: query Site where name or domain == "example.com"
alt match found
Script->>Sites: set Site.domain/name = DASHBOARD_DOMAIN\nsave()
Sites-->>Script: save confirmation
else
Sites-->>Script: no change
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
images/openwisp_dashboard/load_init_data.py (2)
205-205: Usesettings.SITE_IDto reliably identify the default site instead of.first().
Site.objects.first()returns the lowest-PK site, which may not be the site identified bysettings.SITE_IDin multi-site deployments. UseSite.objects.get_current()(which respectssettings.SITE_IDand the request-cache) or the explicit lookup below:♻️ Proposed fix
- site = Site.objects.first() + site = Site.objects.get(id=settings.SITE_ID)You would also need to handle the
Site.DoesNotExistexception ifSITE_IDpoints to a missing record:+ try: + site = Site.objects.get(id=settings.SITE_ID) + except Site.DoesNotExist: + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/load_init_data.py` at line 205, Replace the fragile Site.objects.first() lookup with a SITE_ID-aware lookup: import django.conf.settings and use Site.objects.get_current() (which respects settings.SITE_ID) or Site.objects.get(pk=settings.SITE_ID) inside the same function where site is needed (e.g., around the Site variable assignment in load_init_data.py). Add a try/except for Site.DoesNotExist to handle a missing SITE_ID record and log or raise a clear error, ensuring subsequent code uses the resolved site object.
208-210: Callsite.full_clean()beforesite.save()to validate the env-sourced domain.
dashboard_domaincomes fromos.environwithout any format validation. Callingsite.full_clean()would apply Django's built-inURLValidator/field-level validators on the domain before it is persisted, preventing malformed values from being silently written to the database.♻️ Proposed fix
site.name = dashboard_domain site.domain = dashboard_domain + site.full_clean() site.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/load_init_data.py` around lines 208 - 210, The code assigns dashboard_domain (from os.environ) directly to site.name and site.domain then calls site.save(); instead call site.full_clean() before saving to run Django field validators (including URL/Domain validators) and raise/handle ValidationError for malformed env values—so after setting site.name and site.domain invoke site.full_clean() and only then site.save(), ensuring you catch or log ValidationError for dashboard_domain as needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
images/openwisp_dashboard/load_init_data.pyimages/openwisp_dashboard/openvpn.json
🧰 Additional context used
🪛 GitHub Check: CodeQL
images/openwisp_dashboard/load_init_data.py
[failure] 207-207: Incomplete URL substring sanitization
The string example.com may be at an arbitrary position in the sanitized URL.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 207: The conditional using `"example.com" in [site.name, site.domain]`
should be replaced with explicit equality checks to avoid the CodeQL false
positive; update the if in load_init_data.py so it directly compares site.name
== "example.com" or site.domain == "example.com" (while preserving the existing
`site` and `dashboard_domain` checks) so intent is unambiguous and behavior
unchanged.
In `@images/openwisp_dashboard/openvpn.json`:
- Around line 14-15: Change the incorrect OpenVPN config field and format:
replace the unrecognized "data_cipher" key with "data_ciphers" and convert its
string value into an array of cipher objects (each object containing "cipher"
and "optional" keys) instead of the colon-separated string; also remove the
"none" (unencrypted) option so only valid encrypted ciphers like "AES-128-GCM"
are listed. Ensure the final "data_ciphers" is an array of objects (e.g., {
"cipher": "...", "optional": false }) to match the netjsonconfig v1.1.0+ OpenVPN
schema.
---
Nitpick comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 205: Replace the fragile Site.objects.first() lookup with a SITE_ID-aware
lookup: import django.conf.settings and use Site.objects.get_current() (which
respects settings.SITE_ID) or Site.objects.get(pk=settings.SITE_ID) inside the
same function where site is needed (e.g., around the Site variable assignment in
load_init_data.py). Add a try/except for Site.DoesNotExist to handle a missing
SITE_ID record and log or raise a clear error, ensuring subsequent code uses the
resolved site object.
- Around line 208-210: The code assigns dashboard_domain (from os.environ)
directly to site.name and site.domain then calls site.save(); instead call
site.full_clean() before saving to run Django field validators (including
URL/Domain validators) and raise/handle ValidationError for malformed env
values—so after setting site.name and site.domain invoke site.full_clean() and
only then site.save(), ensuring you catch or log ValidationError for
dashboard_domain as needed.
| "data_cipher": "AES-128-GCM:none", | ||
| "cipher": "AES-128-GCM", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "openvpn.json" -type fRepository: openwisp/docker-openwisp
Length of output: 108
🏁 Script executed:
# Search for netjsonconfig references and schema definitions
fd -e json -e py -e md "netjson" --exec grep -l "data.cipher" {} \;
rg -n "data.cipher|data-cipher" --type py --type json -C 2 | head -100Repository: openwisp/docker-openwisp
Length of output: 415
🏁 Script executed:
# Look for netjsonconfig configuration in the repo
fd -type f -name "*.py" | xargs grep -l "netjsonconfig\|openvpn" | head -10Repository: openwisp/docker-openwisp
Length of output: 238
🏁 Script executed:
# Search for netjsonconfig in the repository
rg "netjsonconfig|data.cipher|data-cipher" -C 3 | head -150Repository: openwisp/docker-openwisp
Length of output: 2262
🏁 Script executed:
# Look for python files that might contain configuration or schema
find . -name "*.py" -type f | xargs grep -l "data_cipher\|data-cipher" 2>/dev/nullRepository: openwisp/docker-openwisp
Length of output: 52
🏁 Script executed:
# Check if there are any netjsonconfig configuration files or documentation
find . -name "*.md" -o -name "*.txt" -o -name "requirements*.txt" | xargs grep -l "netjsonconfig" 2>/dev/null | head -5Repository: openwisp/docker-openwisp
Length of output: 50
🏁 Script executed:
# Look at the openvpn.json file in context to understand the structure
cat -n ./images/openwisp_dashboard/openvpn.json | head -30Repository: openwisp/docker-openwisp
Length of output: 970
🏁 Script executed:
# Search for any documentation or references to netjsonconfig schema
rg "openvpn" -t md -C 3 | head -100Repository: openwisp/docker-openwisp
Length of output: 216
🏁 Script executed:
# Check if there's a setup.py or requirements file that shows netjsonconfig version
cat requirements*.txt setup.py 2>/dev/null | grep -i netjsonconfigRepository: openwisp/docker-openwisp
Length of output: 50
🏁 Script executed:
# Look for any comments or documentation about the openvpn.json file
find . -name "*.md" -o -name "*.rst" | xargs grep -l "openvpn\|data_cipher" 2>/dev/nullRepository: openwisp/docker-openwisp
Length of output: 204
🏁 Script executed:
# Check the load_init_data.py to understand how openvpn.json is used
cat -n ./images/openwisp_dashboard/load_init_data.py | grep -A 20 -B 5 "openvpn"Repository: openwisp/docker-openwisp
Length of output: 3841
🌐 Web query:
netjsonconfig OpenVPN data_cipher data-ciphers field name schema
💡 Result:
In netjsonconfig’s OpenVPN backend schema, the OpenVPN --data-ciphers option is represented by the snake_case field:
data_ciphers: a list of objects, where each object must contain:cipher(string)optional(boolean) (netjsonconfig.openwisp.org)
Related field:
data_ciphers_fallback: a string (maps to OpenVPN--data-ciphers-fallback). (netjsonconfig.openwisp.org)
So the schema field name is data_ciphers (not data_cipher, and not data-ciphers). netjsonconfig added support for this option in v1.1.0. (netjsonconfig.openwisp.org)
Citations:
- 1: https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html?utm_source=openai
- 2: https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html?utm_source=openai
- 3: https://netjsonconfig.openwisp.org/en/master/general/changelog.html?utm_source=openai
🌐 Web query:
netjsonconfig openvpn json schema documentation
💡 Result:
- Primary documentation (OpenVPN backend + schema overview): netjsonconfig “OpenVPN Backend” docs (includes the “OpenVPN backend schema” section and links to the schema source). [1]
- OpenVPN JSON-Schema source (authoritative schema definition used by the backend):
netjsonconfig/backends/openvpn/schema.pyin theopenwisp/netjsonconfigrepo (raw source). [2] - How schemas/validation work in netjsonconfig (general docs): “Basic concepts” (schema inheritance +
validatebehavior). [3] - Project entry point / repo (for browsing other backend schemas and docs versions):
openwisp/netjsonconfigon GitHub. [4] - Underlying NetJSON spec note (why unknown properties may be ignored): NetJSON RFC/draft text describing how unrecognized custom members should be handled. [5]
Sources: [1] (netjsonconfig.openwisp.org) [2] (raw.githubusercontent.com) [3] (netjsonconfig.openwisp.org) [4] (github.com) [5] (netjson.org)
Citations:
- 1: https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html?utm_source=openai
- 2: https://raw.githubusercontent.com/openwisp/netjsonconfig/master/netjsonconfig/backends/openvpn/schema.py
- 3: https://netjsonconfig.openwisp.org/en/latest/general/basics.html?utm_source=openai
- 4: https://github.com/openwisp/netjsonconfig?utm_source=openai
- 5: https://netjson.org/rfc.html?utm_source=openai
🏁 Script executed:
# Check if there's a CHANGES.rst or documentation about field names
cat ./CHANGES.rst | grep -A 5 -B 5 "data.cipher\|netjsonconfig"Repository: openwisp/docker-openwisp
Length of output: 698
🏁 Script executed:
# Look for any validation or schema comments in Python files
rg "data_cipher|data-cipher" -B 3 -A 3Repository: openwisp/docker-openwisp
Length of output: 519
🌐 Web query:
netjsonconfig data_ciphers format list objects cipher optional example
💡 Result:
In netjsonconfig’s OpenVPN backend, the data_ciphers field is:
- a list (array) of objects (dicts)
- each object must contain:
cipher(string)optional(boolean)
as described in the OpenVPN backend schema docs. [1]
(data_ciphers_fallbackis a single string cipher name.) [1]
Example (NetJSONConfig / OpenVPN instance)
{
"openvpn": [
{
"name": "example-vpn",
"mode": "server",
"proto": "udp",
"dev": "tun0",
"dev_type": "tun",
"data_ciphers": [
{ "cipher": "AES-256-GCM", "optional": false },
{ "cipher": "AES-128-GCM", "optional": true }
],
"data_ciphers_fallback": "AES-256-CBC"
}
]
}(Choose cipher names from the OpenVPN backend’s allowed cipher list referenced by the docs.) [1]
Note: data-ciphers support was added to netjsonconfig in v1.1.0 (2024-11-22). [2]
Sources:
[1] netjsonconfig OpenVPN backend docs (schema table including data_ciphers) https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html
[2] netjsonconfig changelog (feature: “Added data-ciphers option for OpenVPN”) https://netjsonconfig.openwisp.org/en/stable/general/changelog.html
🏁 Script executed:
# Check if there are any test files or examples in the repo showing correct format
find . -name "*.json" -o -name "*.py" | xargs grep -l "data_cipher" 2>/dev/nullRepository: openwisp/docker-openwisp
Length of output: 108
Fix field name, format, and remove unencrypted cipher option.
The configuration has three critical issues:
-
Wrong field name:
data_ciphershould bedata_ciphers(plural). With the incorrect name, netjsonconfig silently ignores the entire field, meaning the data cipher configuration is never applied. -
Wrong format: The field expects an array of objects (each with
cipherandoptionalkeys), not a colon-separated string. Current format"AES-128-GCM:none"is invalid for netjsonconfig's OpenVPN schema. -
Security issue: The value
"none"disables data channel encryption and is only for debugging. Shipping it in production allows clients to transmit VPN traffic in cleartext.
Correct the configuration:
Fix
- "data_cipher": "AES-128-GCM:none",
+ "data_ciphers": [
+ {"cipher": "AES-128-GCM", "optional": false}
+ ],(Per netjsonconfig v1.1.0+ OpenVPN backend schema, data_ciphers is a list of cipher objects; data_cipher is not a recognized field and will be silently ignored.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "data_cipher": "AES-128-GCM:none", | |
| "cipher": "AES-128-GCM", | |
| "data_ciphers": [ | |
| {"cipher": "AES-128-GCM", "optional": false} | |
| ], | |
| "cipher": "AES-128-GCM", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/openwisp_dashboard/openvpn.json` around lines 14 - 15, Change the
incorrect OpenVPN config field and format: replace the unrecognized
"data_cipher" key with "data_ciphers" and convert its string value into an array
of cipher objects (each object containing "cipher" and "optional" keys) instead
of the colon-separated string; also remove the "none" (unencrypted) option so
only valid encrypted ciphers like "AES-128-GCM" are listed. Ensure the final
"data_ciphers" is an array of objects (e.g., { "cipher": "...", "optional":
false }) to match the netjsonconfig v1.1.0+ OpenVPN schema.
400f4d0 to
a586871
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
images/openwisp_dashboard/load_init_data.py (1)
205-205: PreferSite.objects.get(pk=settings.SITE_ID)overSite.objects.first().
first()may return the wrong site when multiple sites exist. TheSITE_IDsetting unambiguously identifies the "current" site intended by Django's sites framework.♻️ Proposed refactor
- site = Site.objects.first() + site = Site.objects.get(pk=settings.SITE_ID)Then simplify the guard (the object is guaranteed to exist):
- if site and "example.com" in [site.name, site.domain] and dashboard_domain: + if (site.name == "example.com" or site.domain == "example.com") and dashboard_domain:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/load_init_data.py` at line 205, Replace the fragile query Site.objects.first() with an explicit lookup using Site.objects.get(pk=settings.SITE_ID): import settings if not present, call Site.objects.get(pk=settings.SITE_ID) where the variable site is assigned, and then remove or simplify any subsequent existence guard (since SITE_ID should always resolve to an existing Site); reference the Site model and settings.SITE_ID in the change and ensure any DoesNotExist handling is adjusted or removed accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
images/openwisp_dashboard/load_init_data.pyimages/openwisp_dashboard/openvpn.json
🚧 Files skipped from review as they are similar to previous changes (1)
- images/openwisp_dashboard/openvpn.json
🧰 Additional context used
🪛 GitHub Check: CodeQL
images/openwisp_dashboard/load_init_data.py
[failure] 207-207: Incomplete URL substring sanitization
The string example.com may be at an arbitrary position in the sanitized URL.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🔇 Additional comments (1)
images/openwisp_dashboard/load_init_data.py (1)
255-256: LGTM — placement ofupdate_default_site()is correct.Calling it right after
create_admin()and before VPN setup ensures the site domain is updated before any subsequent steps that might depend on it. The inline conditional import inside the function keeps it self-contained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Around line 136-138: The code unguardedly accesses
template.config['openvpn'][0], which can raise KeyError/IndexError; update the
block around template.full_clean(), template.config and template.save() to
defensively ensure template.config has an 'openvpn' key that is a non-empty list
(e.g., if 'openvpn' missing or not a list, set it to a list with at least one
dict), then set the first element's 'log' value to '/var/log/tun0.log' and call
template.save(); reference the existing symbols template.config,
template.full_clean, and template.save when making this change.
---
Duplicate comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 207: Replace the substring membership check "example.com" in [site.name,
site.domain] with explicit equality comparisons to avoid the CodeQL false
positive: change the conditional that checks site and dashboard_domain (the one
using site.name and site.domain) to test site.name == "example.com" or
site.domain == "example.com" so the intent is unambiguous and substring matching
is eliminated.
---
Nitpick comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 205: Replace the fragile query Site.objects.first() with an explicit
lookup using Site.objects.get(pk=settings.SITE_ID): import settings if not
present, call Site.objects.get(pk=settings.SITE_ID) where the variable site is
assigned, and then remove or simplify any subsequent existence guard (since
SITE_ID should always resolve to an existing Site); reference the Site model and
settings.SITE_ID in the change and ensure any DoesNotExist handling is adjusted
or removed accordingly.
- Updated ciphers - Added login to OpenVPN template
pandafy
force-pushed
the
default-site-and-openvpn
branch
from
a586871 to
1b3ff7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
images/openwisp_dashboard/openvpn.json (1)
14-14:⚠️ Potential issue | 🔴 Critical
data_ciphersstill has the wrong value type, andnoneremains a security risk.The field name was corrected to
data_ciphers(plural), but the value"AES-128-GCM:none"is still a colon-separated string. Per the netjsonconfig OpenVPN schema,data_ciphersexpects a list of dicts, each with acipherandoptionalkey. A plain string is not a valid value and will be treated as an unrecognized type — according to the NetJSON spec, any unrecognized property will be ignored — so this field has no effect at runtime.Additionally,
nonein the cipher list disables data-channel encryption and should never ship in production config.🔧 Proposed fix
- "data_ciphers": "AES-128-GCM:none", + "data_ciphers": [ + {"cipher": "AES-128-GCM", "optional": false} + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/openvpn.json` at line 14, Replace the string value for the data_ciphers field with a list of dicts conforming to the NetJSON OpenVPN schema (each dict must have cipher and optional keys); e.g. remove the insecure "none" entry and use an array like [{"cipher":"AES-128-GCM","optional":false}] so data_ciphers is an actual list of objects rather than a colon-separated string and thus will be honored at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Around line 206-216: Site.objects.get(pk=settings.SITE_ID) can raise
Site.DoesNotExist and never returns None, so remove the dead "if site" guard and
wrap the lookup in a try/except Site.DoesNotExist block; on exception, skip the
update (or log a clear message) instead of letting the init script crash, and
keep the existing update logic (assigning dashboard_domain to site.name and
site.domain, calling site.full_clean() and site.save()) inside the try block so
it only runs when the Site exists and dashboard_domain is set.
- Line 137: The conditional at the top of load_init_data.py redundantly checks
length; replace the expression if template.config.get("openvpn", []) and
len(template.config["openvpn"]) > 0 with a single truthy check using
template.config.get("openvpn") so the code reads simply to test for a non-empty
openvpn list; update any nearby comments to reflect the simplified check.
- Around line 136-139: The code calls template.full_clean(), then mutates
template.config by setting template.config["openvpn"][0]["log"] and immediately
calls template.save() which bypasses validation of the mutated state; update the
flow so the mutation is validated by either moving the openvpn log assignment
before the initial template.full_clean() or, if keeping the current order, call
template.full_clean() again after setting template.config["openvpn"][0]["log"]
and before template.save(); reference the existing symbols
template.full_clean(), template.config["openvpn"][0][\"log\"], and
template.save() when making the change.
---
Duplicate comments:
In `@images/openwisp_dashboard/openvpn.json`:
- Line 14: Replace the string value for the data_ciphers field with a list of
dicts conforming to the NetJSON OpenVPN schema (each dict must have cipher and
optional keys); e.g. remove the insecure "none" entry and use an array like
[{"cipher":"AES-128-GCM","optional":false}] so data_ciphers is an actual list of
objects rather than a colon-separated string and thus will be honored at
runtime.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
images/openwisp_dashboard/load_init_data.pyimages/openwisp_dashboard/openvpn.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🔇 Additional comments (2)
images/openwisp_dashboard/load_init_data.py (2)
261-262: LGTM —update_default_site()is correctly placed aftercreate_admin().
219-242: AI summary inconsistency:update_default_site()is not called insidecreate_default_topology.The AI summary states that
update_default_site()is "invoked aftercreate_admin()in two locations: withincreate_default_topology(vpn)flow." The function at lines 219–242 contains no such call. The only invocation is at line 262 in the__main__block.
| "comp_lzo": "no", | ||
| "auth": "SHA1", | ||
| "cipher": "none", | ||
| "data_ciphers": "AES-128-GCM:none", |
There was a problem hiding this comment.
I think we need to add also data_ciphers_fallback which is the new cipher fallback on newer versions.
pandafy
force-pushed
the
default-site-and-openvpn
branch
from
a777560 to
dff0faf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
images/openwisp_dashboard/openvpn.json (1)
14-15:⚠️ Potential issue | 🔴 Critical
data_ciphers/fallback values are still unsafe and likely schema-invalid.Line 14 uses a colon-delimited string for
data_ciphers, and Line 15 includes:nonein fallback. This can either fail schema validation (depending on netjsonconfig version) or allow plaintext negotiation. Keep only encrypted ciphers in the expected structure.🔧 Proposed fix
- "data_ciphers": "AES-128-GCM:none", - "data_ciphers_fallback": "AES-128-GCM:none", + "data_ciphers": [ + { "cipher": "AES-128-GCM", "optional": false } + ], + "data_ciphers_fallback": "AES-128-GCM",netjsonconfig OpenVPN backend schema: what is the exact JSON type for `data_ciphers` and `data_ciphers_fallback`, and is `none` valid/safe for production data-channel encryption?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/openvpn.json` around lines 14 - 15, The data_ciphers and data_ciphers_fallback entries currently include ":none" and a colon-delimited form which is unsafe and may not match the netjsonconfig OpenVPN schema; update the openvpn.json entries for data_ciphers and data_ciphers_fallback to only list valid encrypted cipher names (remove "none" entirely) and use the schema-expected JSON type (e.g., an array of cipher strings or a single cipher string per the netjsonconfig OpenVPN backend spec), and then validate the file against the netjsonconfig/OpenVPN schema to ensure the fields (data_ciphers, data_ciphers_fallback) are schema-compliant and production-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@images/openwisp_dashboard/openvpn.json`:
- Around line 14-15: The data_ciphers and data_ciphers_fallback entries
currently include ":none" and a colon-delimited form which is unsafe and may not
match the netjsonconfig OpenVPN schema; update the openvpn.json entries for
data_ciphers and data_ciphers_fallback to only list valid encrypted cipher names
(remove "none" entirely) and use the schema-expected JSON type (e.g., an array
of cipher strings or a single cipher string per the netjsonconfig OpenVPN
backend spec), and then validate the file against the netjsonconfig/OpenVPN
schema to ensure the fields (data_ciphers, data_ciphers_fallback) are
schema-compliant and production-safe.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
images/openwisp_dashboard/load_init_data.pyimages/openwisp_dashboard/openvpn.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
🔇 Additional comments (3)
images/openwisp_dashboard/load_init_data.py (3)
127-142: Validation flow after config mutation looks correct.Line 137 validates generated config, Line 139 mutates OpenVPN log, and Line 141 re-validates before save. Good ordering.
204-221:update_default_site()hardens init flow and keeps updates scoped.The
Site.DoesNotExisthandling and explicitexample.comchecks avoid crashing init and prevent unintended overwrites.
267-267: Main-flow invocation point is appropriate.Calling
update_default_site()right after admin bootstrap keeps site-domain initialization deterministic.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
images/openwisp_dashboard/openvpn.json (1)
14-16:⚠️ Potential issue | 🔴 Critical
data_ciphersis still invalidly encoded and includes insecurenone.At Line 14,
data_ciphersshould be a list of objects, not a colon-delimited string. Also,noneshould not be offered in production cipher negotiation.Proposed fix
- "data_ciphers": "AES-128-GCM:none", + "data_ciphers": [ + { "cipher": "AES-128-GCM", "optional": false } + ], "data_ciphers_fallback": "AES-128-GCM", "cipher": "AES-128-GCM",In netjsonconfig OpenVPN backend schema, what is the exact expected JSON type/shape for `data_ciphers`, and is using cipher value `none` recommended for production?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/openvpn.json` around lines 14 - 16, The OpenVPN config is using an incorrect string for data_ciphers and includes the insecure value "none"; change the "data_ciphers" field from the colon-delimited string to the expected JSON array of cipher objects (e.g., a list of strings or objects per the netjsonconfig OpenVPN backend schema) and remove "none" from that list so only safe ciphers are offered; also ensure "data_ciphers_fallback" and "cipher" remain valid single-string cipher identifiers (e.g., "AES-128-GCM") and align with the new data_ciphers array entries to avoid mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@images/openwisp_dashboard/openvpn.json`:
- Around line 14-16: The OpenVPN config is using an incorrect string for
data_ciphers and includes the insecure value "none"; change the "data_ciphers"
field from the colon-delimited string to the expected JSON array of cipher
objects (e.g., a list of strings or objects per the netjsonconfig OpenVPN
backend schema) and remove "none" from that list so only safe ciphers are
offered; also ensure "data_ciphers_fallback" and "cipher" remain valid
single-string cipher identifiers (e.g., "AES-128-GCM") and align with the new
data_ciphers array entries to avoid mismatches.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
images/openwisp_dashboard/openvpn.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Build
|
@coderabbitai we offer "none" for backward compatibility. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
nemesifier
changed the title
nemesifier
changed the title
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
kylemanna/openvpn has not been maintained for last 6 years. Due to this, the OpeVPN container was running OpenVPN 2.4. I have changed the base image to lisenet/openvpn which is based on kylemanna/openvpn. This will allow up to run the latest version of OpenVPN. This patch also fixes the "data_ciphers" field in the OpenVPN configuration which was changed in #566
kylemanna/openvpn has not been maintained for last 6 years. Due to this, the OpeVPN container was running OpenVPN 2.4. I have changed the base image to lisenet/openvpn which is based on kylemanna/openvpn. This will allow up to run the latest version of OpenVPN. This patch also fixes the "data_ciphers" field in the OpenVPN configuration which was changed in #566
kylemanna/openvpn has not been maintained for last 6 years. Due to this, the OpeVPN container was running OpenVPN 2.4. I have changed the base image to lisenet/openvpn which is based on kylemanna/openvpn. This will allow up to run the latest version of OpenVPN. Related to #566
Changed the base image in order to use the latest OpenVPN version available as the old base image used was unmaintained for over 6 years. Related to #566
2 participants
Made Site name configuration and uppdated OpenVPN template