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

Add PHP_SESSION_COOKIE_SECURE #107

Conversation

jaydrogers
Copy link
Member

@jaydrogers jaydrogers added the ⚡️ Enhancement Items that are new features requested to be added. label Dec 30, 2022
@jaydrogers jaydrogers linked an issue Dec 30, 2022 that may be closed by this pull request
@jaydrogers jaydrogers changed the title Add PHP_SESSION_COOKIE_SECURE Draft: Add PHP_SESSION_COOKIE_SECURE Dec 30, 2022
@jaydrogers jaydrogers changed the title Draft: Add PHP_SESSION_COOKIE_SECURE Add PHP_SESSION_COOKIE_SECURE Dec 30, 2022
@jaydrogers jaydrogers marked this pull request as draft December 30, 2022 03:19
@jaydrogers
Copy link
Member Author

jaydrogers commented Dec 30, 2022

@zigazajc007: I have this as a work-in-progress, but I am running into an error.

Problem

  • Setting true for session.cookie_secure via a Docker environment variable doesnt work

Workaround, but then fails

  • If I set it to 1, it works
  • If I change it to 0, it fails

Steps to reproduce

  1. Download my secure-cookie branch from my test app: https://github.com/jaydrogers/docker-php-test-app/tree/secure-cookie
  2. Change the environment variable from 0 to 1

Results

When PHP_SESSION_COOKIE_SECURE = 1

Everything works as expected ✅

Docker Output:
Screenshot 2022-12-29 at 21 16 32

PHP Info:
Screenshot 2022-12-29 at 21 16 48

When PHP_SESSION_COOKIE_SECURE = 0

Container fails to start ❌

Docker Output:

ERROR: [/etc/php/8.1/fpm/pool.d/y-override-php-defaults.conf:227] error while parsing 'php_flag[session.cookie_secure]' : invalid boolean value

Screenshot 2022-12-29 at 21 17 23

Next steps

  • Do you have any thoughts on how we can get this to work? It doesn't seem to like the variables when I pass them through Docker environment variables

@zigazajc007
Copy link

Hello,

I'm not sure if Docker environment variables even support booleans. It will probably just provide a variable in string format.

Did you tried to use On and Off instead?

@jaydrogers
Copy link
Member Author

I tried both of those as well.

If we are unable to change this to a Boolean, then the other other option I see is changing it from php_admin_value to php_value so it can be set by PHP.ini.

The only thing I don't like about that is it could potentially open up a security issue if someone is able to get the server to dump a rogue PHP.ini and ignore secure cookies.

Thoughts?

@zigazajc007
Copy link

I would personally provide image with php.ini that has most of security features turned on by default. So the ones that doesn't know how to customize php.ini then it shouldn't impact security if they use default php.ini that you provide.

But for those who knows how to edit php.ini and are willing to provide their own then I wouldn't force settings in some hidden .ini files that overwrite the default php.ini

Advanced developers would think that your image is broken if they try to turn off secure cookies and it won't work.

So I will just make sure that the default php settings are secure, but still leave developers to make any change with ease.

@jaydrogers
Copy link
Member Author

Another option is to leave it as-is, then someone can add a zzz config file that will be loaded after mine: https://github.com/serversideup/docker-php/tree/main/src/fpm/etc/php/fpm/pool.d

@zigazajc007
Copy link

What if you check if user did set SECURE_COOKIES in env variable to false, Off or 0 and if so, then you can just append new line in your .conf file with echo?

@zigazajc007
Copy link

It seems to be only working if I delete this line from the file:

RUN sed -i '/session.cookie_secure/d' /etc/php/current_version/fpm/pool.d/y-override-php-defaults.conf

@jaydrogers
Copy link
Member Author

This PR is being closed in favor for the fix being applied in the v3 of the Docker images (coming soon).

It was resolved in this commit, where I moved the config out of PHP-FPM and set it within the PHP.ini: c8d848a

Status of the v3 is located here: #207

@jaydrogers jaydrogers closed this Oct 26, 2023
jaydrogers added a commit that referenced this pull request Feb 5, 2024
* Improved query accuracy

* Removed sort

* Removed version data

* Changed docker repo

* Restructured docker repository name

* Moved readme control to beta for now

* Improved error handling

* Removed unset check

* Added dev script

* Removed copy command

* Fixed merging of YML

* Disabled debugging

* Fixed merge

* Remove debug

* Improved dependency install

* Fixed structure of dev command

* Improved documentation

* Initial commit of S6

* Initial commit of other variations

* Added early exits

* Fixed separation of commands

* Added init script

* Removed whitespace

* Sort scripts to run by numerical order

* Fixed execution of commands

* Improved development script

* Removed debug

* WIP migrated FPM and NGINX over

* Removed wget dependency

* Fixed reference to php-fpm

* Changed shells

* Fixed dependency installer

* WIP - adding nginx

* Converted logic to shell

* Added trace

* Working NGINX

* Working NGINX-FPM

* Working Alpine support

* Functioning Alpine image

* Added special thanks

* Cleaned up error output

* Removed whitespace

* Fixed error reference

* Remove unnecessary code

* WIP Nginx Unit

* Set script name

* Renamed nginx-unit to unit

* Fixed base image reference

* Improved error output

* Working NGINX Unit with SSL-Off

* Cleaned up output

* Explicit script names

* Added debug

* WIP Unit Mixed

* Renamed SSL to self-signed

* Working SSL configuration

* Changed to serversideup/php

* Changed docker-php-pro to docker-php-serversideup

* Fixed illegal character error

* Added minimum TLS

* Updated ciphers

* Working NGINX Unit configuration

* Changed name reference

* Changed branch name for beta

* Added usage notes

* Refactored tag script

* Renamed base config file

* Renamed base config

* Moved RC and added usage instructions

* Removed whitespace

* Removed whitespace

* Require base config

* Clarified defaults

* Renamed DEPENDENCY_PACKAGES_BOOKWORM to DEPENDENCY_PACKAGES_DEBIAN

* Refactored and improved readability

* Added base operating systems

* Updated php-version.yml structure

* Added base os for RC

* Refactored assemble-docker-tags

* Adjusted Base OS Logic

* Fixed variable name

* Variable name change

* Pretty print

* Changed permissions

* Added help menu

* Fixed help menu

* Remove help tag

* Added fpm configs

* Added default variables

* Organized and renamed variables for better clarity

* Remove PHP open base dir

* Fixed permissions on configs

* Moved PHP_SESSION_COOKIE_SECURE to ini. Fixes #105. Ref #107

* Moved init scripts to be at a higher number

* Added PHP_ERROR_LOG

* Changed order of execution for debug

* Changed debug output order

* Removed default base os

* Prepared for push of images

* Default to any debian os

* Updated landing page

* Updated readme

* Improved NGINX Unit description

* Updated features

* Improved output

* Added newer versions

* Added build process

* Removed fpm-apache for now

* Updated NGINX Unit version to 1.31.1

* Added variations from config file

* Fixed tagging issues for default_base_os

* Added comment for why we can't include an alpine version of Unit

* Ignore DS_Store files

* Added PHP extensions

* Removed whitespace

* Commented out sqllit

* Specified branch due to cloudflare/pages-action#97

* Update s6-overlay version to v3.1.6.2

* Updated comparison and moved S6 overlay to its own page

* Added APP_BASE_DIR

* Clarified release notes location

* Started migration guide

* Removed whitespace

* Update migration guide with v3 changes

* Updated Laravel automations

* Fixed heading reference

* Add branch parameter to Cloudflare deployment

* Cleaned up GitHub Action names

* Added skip-download option

* Temporarily set skip download

* Add expanded Laravel automations

* Clarified output

* Fix autorun Laravel migration to true

* Update PHP Docker Images and Add New Features

* Clarified comment

* Enabled downloads again

* Add PHP 8.3 with Alpine and Bookworm support

* Default to 8.3

* Added debug mode

* Moved debug mode to entrypoint

* Update Dockerfile and entrypoint scripts

* Added labels and ability to set repo versions

* Adjusted argument order

* Update environment variable specification

* Update REPOSITORY_BUILD_VERSION to include GIT
Short SHA and GitHub Run ID

* Update Dockerfile comments for CLI and FPM
variations

* Update PHP Docker Images description

* Add explanation for "Optimized for Laravel &
WordPress" in getting started guide

* Simplified checkout process

* Added ability to push to GHCR

* Add multi-arch support and published registry URLs

* Remove Discourse link

* Updated installation instructions

* Update upgrade guide with version selection
details

* Add links to view available images and tags

* Update installation.md with information about
selecting the version of PHP

* Update link to guide on selecting the right image

* Updated contribution guide

* Update docker-compose.yml to include .dockerignore
file

* Add composer support

* Set S6 Overlay version in variable instead of file

* Update project credits and special thanks

* Update title in marketing layout

* Adjusted default PHP extensions

* Added $build_major_version-$build_base_os tag

* Add default entrypoint scripts and disable default
configuration

* Fix typo in default configurations

* Add guide for migrating from official PHP Docker
images

* Fix missing image tag and add link to installation
guide

* Update PHP image migration guide

* Add default environment variable specification

* Add guide on changing common PHP settings

* Remove customizing the image guide

* Add migration guide from v2 to v3

* Update Docker Compose and add Dockerfile for
installing additional PHP extensions

* Fix image tag not found issue in older versions of
PHP

* Add instructions for rebuilding Docker image on
Docker Compose initialization

* Add install-php-extensions support

* Refactor Docker tag generation logic

* WIP of SSL and start up scripts

* Docs Upgrade (#245)

* Upgraded to Nuxt 3.8

* Added about images and content ref #238

* Updated to H1 Title

* Removed methods for SEO data

* Added OG Image template

* Cleaned up SEO

* Fixed undefined page variable

* Added sign up form

* Added nvm

* Add Node.js setup and update build process

* Update link to customizations guide

* Update environment name in marketing site preview
workflow

* Updated to base 64

* Update environment name in
action_marketing-site-publish.yml

* Add BASE_PATH to .env.example

* Add SSL configuration options

* Update SSL configuration options

* Update ogImage and twitterImage URLs in marketing
layout

* Allow for nuxt base url to be null

* Adjusted w and h of image

---------

Co-authored-by: Jay Rogers <jay@521dimensions.com>

* Update SSL configuration

* Updated logo

* Update PHP patch versions

* Fixed code highlights

* Update SSL configuration and provide examples for
self-signed and custom certificates

* Add note about php.ini in
changing-common-php-settings.md

* Update Dockerfile versions and install additional
PHP extensions

* Add guide for adding custom startup scripts

* Organized variables better

* Update migration guide with v3 changes

* Update PHP installation instructions and remove
deprecated features

* Removed arrow for compatibility reasons

* Update link to discussion

* Update PHP Docker Images with improved
customization

* Added PHP 8.3 support

* Update description in index.md file

* Update description of serversideup/php Docker
images

* Update PHP Docker Images name

* Changed to PNG image

* Update ogImageHeight in marketing layout

* Update ogDescription and twitterDescription in
marketing layout

* Update social image file type

* Update social image

* Adjusted OG image template

* Adjusted borders

* Update border and rounded styles in OgImage
component

* Update command reference in PHP Docker image

* Updated favicon Url ref #251

* Add PHP_OPEN_BASEDIR environment variable to Dockerfiles. Fixes #258

* Add port exposure for HTTP and HTTPS. Fixes #260

* Add PHP_MAX_INPUT_TIME environment variable. Fixes #257

* Added ZIP to Debian images. Fixes #261

* Add WORKDIR instruction. Fixes #263

* Update environment names for marketing site workflows

* Test CI

* CI test 2

* Revert environment names

* Cleaned up environment names

* CI test 1

* Remove testing

* Update PHP_EXT_INSTALLER_VERSION to 2.1.71

* Update PHP_EXT_INSTALLER_VERSION to 2.1.73

* Update Docker actions to latest versions

* Add prefix option for Docker image tag

* Add composer cache directory creation and ownership

* Added docker-php-serversideup-set-id script

* Removed --chown flag. Fixes #265

* Added video embed component

* Added Book & updated header

* Fix typo

* Fixed code execution

* Improved s6-init build process

* Added Dockerfile support

* Added S6 overlay script customization

* Add book recommendation for building multi-platform browser extensions

* Update menu link to Bugflow and change "Books" to "Products"

* Added automated checks to ensure database connection is alive before running migrations

* Added AUTORUN_LARAVEL_MIGRATION_TIMEOUT. Default timeout of 30 seconds

* Update PHP_EXT_INSTALLER_VERSION to 2.1.76

* Improved test connection to database. Fixes #279

* Upgraded to actions/checkout@v4

* Prepare for merge

* Update PHP patch versions in base config

---------

Co-authored-by: Dan Pastori <dan@521dimensions.com>
@jaydrogers jaydrogers deleted the 105-add-another-variable-that-allows-to-toggle-sessioncookie_secure branch April 22, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡️ Enhancement Items that are new features requested to be added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add another variable that allows to toggle session.cookie_secure
2 participants