Devsecops/GitHub policies (#286)

* Update source control policy

* Add repo setup to policy

* Fix linting issues

* Update based on feedback

* Update to have Azdo information

software-engineering-policies/SourceControl/AzDoBranchProtection.md

@@ -0,0 +1,45 @@
+# Branch Protection
+
+Branch protection on active repositories is important to ensure that we are doing the best we can to be clean and secure. The policies set out below are required for your main branch but options for other branches.
+
+The following options should be selected. An explanation is provided where necessary as to why this should be enabled.
+
+## Branch Policies
+
+The following policies should be enabled against a repository to ensure that we are adhering to our source control policies and being as vigilant as possible when it comes to code reviews prior to committing our code to main.
+
+### Require a minimum number of reviewers
+
+The team size will dictate the number of approvals required to move a pull request through the process but the ideal here should be 2. This allows for multiple views of the code from different angles to make sure what you are trying to merge fits what is required. However it is understood sometimes team sizes are not large enough for this so an absolute minimum would be 1 reviewer.
+
+#### Prohibit the most recent pusher from approving their own changes
+
+Ideally anybody who is contributing to the pull request should be getting an external review so enabling this will help us prevent developers making "small fixes" before approving and merging.
+
+#### When new changes are pushed: Reset all code reviewer votes (does not reset votes to reject or wait)
+
+Any new change indicates that the pull request needs a review from all people. However if there has been a request to reject or wait there should be a chance for these individuals to double check their concerns have been addressed before approving.
+
+### Check for linked work items: Optional
+
+Whilst it would be ideal to have a work item attached to each pull request, there will be times where a work item cannot be attached for one reason or another. Having the warning will allow us to think about if we should have the work item when creating the pull request.
+
+### Check for comment resolution: Required
+
+Having this set to required allows anybody who has commented to ensure that their comment has been seen and/or dealt with before a pull request has been actioned. If there are problems later in the development process then the pull request can be referenced back to and there should be no loose conversations.
+
+### Limit merge types
+
+The preference here is to use Squash merge so that we can have a linear history and allow for multiple commits (including any errors) in a pull request without it reflecting in the main branch. However this is a team choice and the above is guidance only.
+
+## Build Validation
+
+To ensure that what is being put into main meets expectation we should be putting build validation gates in place. Wherever there is automation to test the codebase or code changes (such as Snyk or Trivvy) then we should be adding these pipelines as a policy. This should trigger automatically whenever a change is made and should expire whenever the main branch is updated.
+
+## Status Checks
+
+Status checks are the automated checks that run on the code to support in quality and security. Requiring these is a must but it is not enough to just turn this on, you need to be explicit in which checks you want requiring.
+
+### Automatically Included reviewers
+
+The code owners should be the gate keepers of the repository and no code should pass this point without their knowledge or approval. It is the code owners who have the knowledge of the code and its purpose that can help guide a PR so having this is extra important. 

software-engineering-policies/SourceControl/GitBranchProtectionPolicy.md

@@ -0,0 +1,46 @@
+# Branch Protection
+
+Branch protection on active repositories is important to ensure that we are doing the best we can to be clean and secure. The policies set out below are required for your main branch but options for other branches.
+
+The following options should be selected. An explanation is provided where necessary as to why this should be enabled.
+
+## Require a pull request before merging
+
+We should never be making any code changes to the main branch without making a pull request. This can lead to a messy history, conflicts and broken builds that can cause outages. By performing a pull request, your code can be put through quality checks, code review and even be run in an environment before being merged.
+
+### Require approvals
+
+The team size will dictate the number of approvals required to move a pull request through the process but the ideal here should be 2. This allows for multiple views of the code from different angles to make sure what you are trying to merge fits what is required. However it is understood sometimes team sizes are not large enough for this so an absolute minimum would be 1 reviewer.
+
+### Dismiss stale pull request approvals when new commits are pushed
+
+If a code change is made, no matter how tiny, it is best that your reviewers take another look just to be sure that everything is ok. It may seem overkill for something like a line deletion or missing semicolon however it stops code creep and ensures you are proofing your code prior to reviews in the future.
+
+### Require reviews from Code Owners
+
+The code owners should be the gate keepers of the repository and no code should pass this point without their knowledge or approval. It is the code owners who have the knowledge of the code and its purpose that can help guide a PR so having this is extra important.
+
+
+## Require status checks to pass before merging
+
+Status checks are the automated checks that run on the code to support in quality and security. Requiring these is a must but it is not enough to just turn this on, you need to be explicit in which checks you want requiring.
+
+### Require branches to be up to date before merging
+
+It is pointless running checks and doing code reviews on code that is not inline with your main branch. It could be that your code is fine however there has been a change made that conflicts with yours that would not be found until merging. This check allows us to prevent this from happening.
+
+### Snyk SAST status check to be required
+
+Snyk has been setup against all Repos. This means when you enable status checks to be required and you search for the status checks you want to enable, typing `snyk` into the text box provided should hopefully show `snyk/sast` and you can select it. It is important that we have this as a required check as we do not want to push insecure code into main. 
+
+## Require conversation resolution before merging
+
+If there are conversations on your pull request then it is important to know that these have been acknowledged in one way or another. Sometimes a passing comment can go unnoticed but could have an effect on something down the line. By dealing with it at the pull request we can be sure that the conversation is at least taken into consideration.
+
+## Do not allow bypassing the above settings
+
+The point of these settings is to ensure we deliver safe and secure code where possible. By allowing anyone to bypass this would defeat the object of what we are trying to achieve. Whilst a nuisance, they provide a level of automated checks to help with the continued delivery of our code, so problems should be dealt with at the various levels and not bypassed. There will always be unique cases where this cannot be done but that must be dealt with on a case by case basis.
+
+## Restrict who can push to matching branches
+
+As we work with 3rd parties and multiple teams, sometimes having codeowners reviewing is not enough and we need to actually restrict who is pushing to the branch. For main it should be the service owners and any other teams who are maintaining the service. 

software-engineering-policies/SourceControl/RepositorySetupPolicy.md

@@ -0,0 +1,69 @@
+# Repository Setup Policy
+
+To ensure consistency in how our repositories are setup for access and automation, the following policy has been created. This focuses on the files within the codebase, the settings within the repo and automation we expect to run. 
+
+## Codebase setup
+
+So that we can ensure that anybody interacting with the codebase knows how they can perform certain actions, the following files should be added.
+
+### CODEOWNERS
+
+The CODEOWNERS file uses pattern matching to figure out who should be reviewing what when it comes to a pull request. For a CODEOWNERS file to be valid, the users/teams must have write permission to the repository. More information on how this can be setup and structured can be found in the [github documentation](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners)
+
+### LICENSE
+
+This describes the license in which the codebase is under. 
+
+### CONTRIBUTING.md
+
+The contributing file explains to anyone wishing to interact with the codebase how they would go about doing so. This would include raising issues, how to go about creating pull requests and any guidance to what should be included when making changes. Examples of these files can be found [here](/using-github/creating-a-contributing-file.md)
+
+### SECURITY.md
+
+The security file provides a way for people interacting with the codebase to know how to raise any security issues they may have found. We have a fairly standard format for this that you can add to the file 
+
+```md
+# Security Notice
+
+The UK Hydrographic Office (UKHO) supplies hydrographic information to protect lives at sea. Maintaining the confidentially, integrity and availability of our services is paramount. Found a security bug? Please report it to us at [email protected]
+```
+
+### README.md
+
+The readme is the window to the codebase. This should have a simple explanation of what the code does, how to run the code/tests and any additional information that is pertinent to the codebase. If there is extensive documentation around the codebase due to its size, breaking this up into smaller documents and having them in a docs folder is usually good practice with links out. 
+
+## Settings
+
+Setting up the repository is the key to ensuring we keep things clean and ensuring we are doing the best to implement the policies provided.
+
+### Default Branch
+
+As stated in the Source Control policy, we should be using `main` as our default branch where possible. For new repositories this should not be a problem and should only be an issue where we may have legacy repos and pipelines.
+
+### Features
+
+So that we are only using the features that we require of Github and avoid any confusion of where information needs to be, we should only have `Preserve this repository` enabled. In terms of the other features we use other tooling for these so by disabling them we remove possible interactions.
+
+### Pull Requests
+
+There are multiple merging strategies for merging a pull request into a repository.
+
+- Merge commits allow us to merge all commits.
+- Squash and merge puts all commits into 1 and is added to the base branch.
+- Rebase and merge puts all commits into 1 and is rebased onto the base branch.
+
+Because of how we are setting up with our branch protection and other settings, the ideal merge strategy here would be `Squash and merge`. This gives us the room to create a pull request with multiple commits and "mistakes" but the main branch is kept clean with a single commit from the pull request. The commit message can contain a summary of everything that has been completed and the PR itself can be the history. 
+
+It would be advised to disable the other 2 merge strategies so that teams are forced to use the same way of merging however this can be left up to teams to discuss and decide. 
+
+Enabling `Always suggest updating pull request branches` is required as we never want to be behind the main branch when we are performing pull requests.
+
+Finally `Automatically delete head branches` is required to be enabled to support in cleaning up branches. This also supports in re-pointing branches using the deleted head branch as a base.
+
+## Branch Protection Rules
+
+Branch protection should always be setup against your default branch. The policy for this can be found [here](/software-engineering-policies/SourceControl/BranchProtectionPolicy.md)
+
+## Code security and analysis
+
+From a code security point of view, we already have tooling that runs against the codebase externally however to support in the maintenance of your codebase we should be enabling `Dependabot alerts`, `Dependabot security updates` and `Dependabot version updates` so that we can try and keep on top of dependency updates and vulnerability updates.

software-engineering-policies/SourceControl/SourceControlPolicy.md

@@ -1,6 +1,6 @@
 # Source Control
 
-* All code **must** be stored in a source code repository.
+* All UKHO code **must** be stored in a source code repository.
 * Git is the preferred method of source control.
 * If hosted at the UKHO, these must be used on the central Azure DevOps Server system.
 * It is acceptable to use our official Azure DevOps instance on Azure to provide version control.
@@ -9,34 +9,50 @@
 * Any source code found in any other source control method must first be migrated and proven to build via a CI pipeline.
 * All new source will host a primary branch called _trunk_ or _main_ over _master_.
 
+## Repository Setup
+
+Information about setting up your repository can be found [here](/software-engineering-policies/SourceControl/RepositorySetupPolicy.md)
+
 ## Access control
 
-Ensure that developers with BPSS clearance are only granted read access.  BPSS-cleared developers may temporarily be granted write access to a repository, provided suitable controls exist to prevent any change they make progressing to a live environment without review by a Security Cleared (SC) colleague.
+Access to a repo should be done through a team and not through an individual as to ensure that there is never a single point of failure. Team access does not mean team ownership however.
+
+Ensure that developers with BPSS clearance are only granted read access. BPSS-cleared developers may temporarily be granted write access to a repository, provided suitable controls exist to prevent any change they make progressing to a live environment without review by a Security Cleared (SC) colleague.
 
-## Branch Protection
+## Branching
 
-Suitable branch protection should be setup, depending on factors agreed by the team, to avoid merging to main.
+As a Software Engineering collective, we endeavour to practice [github flow based development](https://docs.github.com/en/get-started/using-github/github-flow) when working within our teams and when working with 3rd parties.
 
-Some suggestions are:
+To be able to trace our branches back to PBI's it is preferable that we keep out branch naming consistent. The ideal would look something like 
 
-* using the code owner file
-* ensuring a team is associated as an admin on the repository
-* assigning the team as the only members who can merge
-* apply to administrators
-  * A team member who is an admin on the repository could bypass the protection
+`{ticket_number}/{type}-{description}`
 
-## Branching
+So for example if someone was working on a feature to update a button the branch name could look like
 
-Branching policies and schemes are decided at the team level, according to that team's needs.
+`1001/feature-update-button-onclick`
 
-Branching policy and naming conventions are the responsibility of the lead engineers and must be published to the team.
+Branch Protection should be setup against the main branch. Information is provided for [GitHub](/software-engineering-policies/SourceControl/GitBranchProtectionPolicy.md) and [Azure Devops](/software-engineering-policies/SourceControl/AzDoBranchProtection.md)
 
 ## Check-in comments
 
 All check-ins must be accompanied by a comment.  This should be enforced by the tooling.  The check-in comments should be subject to review as part of code review.
 
 The commits should be small and concise and the commit message should answer "Applying this change will...".
 
+## Pull Requests
+
+Pull requests should follow a consistent pattern to help with reviewing. There are 3 parts to a pull request that need to be considered.
+
+### Naming
+
+The naming for a pull request should be in a similar fashion to the branch name where the ticket, type and description is provided. For example
+
+`1001 - Feature Update button OnClick`
+
+The body of a pull request should provide the information and context a reviewer would need to proceed with looking through you code. This will include what has changed, why you changed it, any tickets relating to this change and how to test this change. If possible providing a template for this will support in ensuring that a developer will add the required information when raising a pull request.
+
+The contents of the PR should be specific to the work being described in the pull request. If there is a ticket attached then the changes should be specific to that and any deviations called out explicitly. PR sizes should not be too large as to not overwhelm your reviewers; where they do become too large a code review before hand might be of benefit.
+
 ## Verification
 
 A team's source control system must be open to inspection, including history of check-in comments.